Fixed a number of memory allocation issues that rendered the component unuseable #4

Closed
wants to merge 26 commits into
from

Conversation

Projects
None yet
4 participants
@ruipacheco

I'm not sure of some of the work I did was rolled back or if some of the changes you did exposed bug in the code. Either way, I've added a number of dealloc and retain messages to the code. It was crashing randomly on my project and seems solid now.

As an aside, this is one of the most confusing code bases I've seen. Needs a good rewrite, might take that on as a summer project.

2011/10/08

Cleared up some of the retain noise for the benefit of other users who also need the component.

@mugginsoft

This comment has been minimized.

Show comment Hide comment
@mugginsoft

mugginsoft Aug 3, 2011

Hi Rui

Some of the above is wrong. For instance:

sharedInstance = [[[self alloc] init] retain];

The memory management rules state that you own an object you allocate so the retain is unnecessary.

I didn't rollback any of your changes but was directed by the clang analyser to fix what seemed like leaks.
This could have exposed some errors in your code.

Glancing at the diff I would say that some of the accessors probably do need retains as you have shown.
But there is too much retain noise to merge it as is.

I don't use RC at the moment but I think I will try and find time to build the test app as RC and see where the issues are.

As for the code base it's less than stellar. A good refactoring would be beneficial.

Hi Rui

Some of the above is wrong. For instance:

sharedInstance = [[[self alloc] init] retain];

The memory management rules state that you own an object you allocate so the retain is unnecessary.

I didn't rollback any of your changes but was directed by the clang analyser to fix what seemed like leaks.
This could have exposed some errors in your code.

Glancing at the diff I would say that some of the accessors probably do need retains as you have shown.
But there is too much retain noise to merge it as is.

I don't use RC at the moment but I think I will try and find time to build the test app as RC and see where the issues are.

As for the code base it's less than stellar. A good refactoring would be beneficial.

@rogergilblat

This comment has been minimized.

Show comment Hide comment
@rogergilblat

rogergilblat Oct 5, 2011

Is there a ETA on making this work in a non-GC environment? I'm trying to integrate this into my non-GC project and it's having problems. I spent a few hours last night trying to fix the Fragaria code, but was getting lost. It might be easier to convert my code to GC. Wanted to see how far out this fix is before doing that.

Is there a ETA on making this work in a non-GC environment? I'm trying to integrate this into my non-GC project and it's having problems. I spent a few hours last night trying to fix the Fragaria code, but was getting lost. It might be easier to convert my code to GC. Wanted to see how far out this fix is before doing that.

@ruipacheco

This comment has been minimized.

Show comment Hide comment
@ruipacheco

ruipacheco Oct 6, 2011

Hello,

We've fixed it, or at least I have it running on my project. There are some
issues with retain cycles but those should be fixed soon. I'll try to push
something by Saturday night.

On 6 October 2011 01:39, rogergilblat <
reply@reply.github.com>wrote:

Is there a ETA on making this work in a non-GC environment? I'm trying to
integrate this into my non-GC project and it's having problems. I spent a
few hours last night trying to fix the Fragaria code, but was getting lost.
It might be easier to convert my code to GC. Wanted to see how far out
this fix is before doing that.

Reply to this email directly or view it on GitHub:
#4 (comment)

Best regards,
Rui Pacheco

Hello,

We've fixed it, or at least I have it running on my project. There are some
issues with retain cycles but those should be fixed soon. I'll try to push
something by Saturday night.

On 6 October 2011 01:39, rogergilblat <
reply@reply.github.com>wrote:

Is there a ETA on making this work in a non-GC environment? I'm trying to
integrate this into my non-GC project and it's having problems. I spent a
few hours last night trying to fix the Fragaria code, but was getting lost.
It might be easier to convert my code to GC. Wanted to see how far out
this fix is before doing that.

Reply to this email directly or view it on GitHub:
#4 (comment)

Best regards,
Rui Pacheco

@mugginsoft

This comment has been minimized.

Show comment Hide comment
@mugginsoft

mugginsoft Dec 10, 2012

Owner

RC is now supported.

Owner

mugginsoft commented Dec 10, 2012

RC is now supported.

@mugginsoft mugginsoft closed this Dec 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment