Skip to content

Commit

Permalink
Memory management.
Browse files Browse the repository at this point in the history
  • Loading branch information
Jasper Blues committed Mar 4, 2013
1 parent d13ba1a commit e9f5ee3
Showing 1 changed file with 11 additions and 4 deletions.
15 changes: 11 additions & 4 deletions Source/Factory/InstanceBuilder/TyphoonComponentFactory+InstanceBuilder.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ @implementation TyphoonComponentFactory (InstanceBuilder)
/* ========================================================== Interface Methods ========================================================= */
- (id)buildInstanceWithDefinition:(TyphoonDefinition*)definition
{
id <TyphoonIntrospectiveNSObject> instance;
__autoreleasing id <TyphoonIntrospectiveNSObject> instance;

if (definition.factoryReference)
{
Expand Down Expand Up @@ -83,9 +83,16 @@ - (id)invokeInitializerOn:(id)instanceOrClass withDefinition:(TyphoonDefinition*
}
}
[invocation invoke];
__autoreleasing id <NSObject> returnValue = definition.initializer.isClassMethod || definition.factoryReference ? nil : instanceOrClass;
[invocation getReturnValue:&returnValue];
return returnValue;
if (definition.initializer.isClassMethod || definition.factoryReference)
{
__autoreleasing id <NSObject> returnValue = nil;
[invocation getReturnValue:&returnValue];
return returnValue;
}
else
{
return instanceOrClass;
}
}

/* ====================================================================================================================================== */
Expand Down

9 comments on commit e9f5ee3

@IvanUshakov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have bug introduced in this commit. And I would ask, for what it's made?

@jasperblues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this method we're just calling the initializer/creator for the component. . . It could be in the folliwing form:

Instance Methods:

  • init
  • initWithX:Y:Z

Class Methods

  • object
  • objectWithX:Y:Z

The first change puts the returned instance in an autorelease pool. . . I don't recall why I added this.

The second change is just for readability.

@IvanUshakov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for answer and sorry, I wrong find commit.
I have bug with returning not initialized object. This occurs, because we have condition:
if (definition.initializer.isClassMethod || definition.factoryReference)
And I can't understand for what this made.

@jasperblues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That if statement does the following:

  • If the creation method is a class method - eg: objectWithX:Y:Z or its using another component to produce this component
    . . . then start with nil, and fetch the result of the invocation into that and return it.
  • Otherwise for instance methods (eg init/initWith) just return, because we've already performed the invocation, which would modify the object we pass into the invocation. . . . (Except it seems for some cases where there's a proxy object involved, eg with CoreData. . . A user mentioned they had some problems with CoreData - this issue is still open. . . your issue may be related).

@IvanUshakov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my issue is related. Not only Core Data, but some other classes make that. And last question, if I remove this condition, all will work fine?

Now I make injecting parameters by raw value. Did you interested to that?

@jasperblues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No, I don't think removing that will fix the problem. . . it would certainly help if you try and let me know.

I'm planning to change the Data Access Objects in the Typhoon example application to use Core Data. . . this should then break it, and provide a Test Bed to fix the problem. . I haven't gotten around to doing this yet - part of the problem is that I don't have much experience with Core Data. . . would you be interested in doing that? (Changing the example to use CoreData, and hence breaking it)

  1. Contributions are always welcome.

@IvanUshakov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fix that problem, and all test done ok, but I'm not sure, that this have not side effects.

I'm too have small experience in core data. But in my opinion, feature that I do can make using core data with typhoon easier.

@jasperblues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean all of the Typhoon tests? The coverage there is pretty good, so it should be good to go. Nice work.

@IvanUshakov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, Then I'm going to do PR now.

Please sign in to comment.