CODEWITHSUNDEEP
objective-carrayspointersmemory-leaks
Mathieu
Mathieu

Reputation: 1175

Array of pointers causes leaks

-(void)setUserFilters{

    //init the user filters array
    userFilters = [[NSMutableArray alloc] init];
    SearchCriteria *tmpSc= [[SearchCriteria alloc] init];
    for(int i=0;i<[searchFilters count];i++)
    {
    tmpSc=[self.searchFilters objectAtIndex:i];
    if(tmpSc.enabled==TRUE)
        [userFilters addObject:tmpSc];
    }
    }

searchFilters is a list of filters that can be setted to true or false and I use userFilters to populate a table view with the filters that are only setted to TRUE

But the line SearchCriteria *tmpSc= [[SearchCriteria alloc] init]; causes leaks, and I don't know how to solve because if I release at the end of the function I loose my pointers and it crashes

Any ideas?

Upvotes: 1

Views: 310

Answers (3)

user138336
user138336

Reputation:

It seems that your initially creating a SearchCriteria object and before you use it or release it your reassigning the variable to another object from self.searchFilters. So you don't need to create the initial object and why it's leaking and not being released.

Try:

SearchCriteria *tmpSc = nil;

Hope that helps.

Upvotes: 2

Rob Napier
Rob Napier

Reputation: 299325

twolfe18 has made the code >much slower if searchFilters can be large. -objectAtIndex: is not a fast operation on large arrays, so you shouldn't do it more than you have to. (While true that FE is faster than objectAtIndex:, this overstated the issue and so I've striken it; see my other comments on the advantages of Fast Enumeration.)

There are a number of problems in your code:

  • Never create a method that begins "set" but is not an accessor. This can lead to very surprising bugs because of how Objective-C provides Key-Value Compliance. Names matter. A property named userFilters should have a getter called -userFilters and a setter called -setUserFilters:. The setter should take the same type that the getter returns. So this method is better called -updateUserFilters to avoid this issue (and to more correctly indicate what it does).

  • Always use accessors. They will save you all kinds of memory management problems. Your current code will leak the entire array if -setUserFilters is called twice.

  • Both comments are correct that you don't need to allocate a temporary here. In fact, your best solution is to use Fast Enumeration, which is both very fast and very memory efficient (and the easiest to code).

Pulling it all together, here's what you want to be doing (at least one way to do it, there are many other good solutions, but this one is very simple to understand):

@interface MyObject ()
@property (nonatomic, readwrite, retain) NSMutableArray *userFilters;
@property (nonatomic, readwrite, retain) NSMutableArray *searchFilters;
@end

@implementation MyObject
@synthesize userFilters;
@synthesize searchFilters;

- (void)dealloc
{
    [searchFilters release];
    serachFilters = nil;
    [userFilters release];
    userFilters = nil;
    [super dealloc];
}


- (void)updateUserFilters
{
    //init the user filters array
    // The accessor will retain for us and will release the old value if
    // we're called a second time
    self.userFilters = [NSMutableArray array];

    // This is Fast Enumeration
    for (SearchCriteria *sc in self.searchFilters)
    {
        if(sc.enabled)
        {
            [self.userFilters addObject:sc];
        }
    }
}

Upvotes: 5

twolfe18
twolfe18

Reputation: 2266

first of all, the worst n00b code you can write involves if(condition==true) do_something(), just write if(condition) do_something().

second, there is no reason to have tempSc at all (never mind alloc memory for it), you can just do the following:

-(void)setUserFilters{

//init the user filters array
userFilters = [[NSMutableArray alloc] init];
for(int i=0;i<[searchFilters count];i++)
{
    if([self.searchFilters objectAtIndex:i].enabled)
            [userFilters addObject:[self.searchFilters objectAtIndex:i]];
}
}

Upvotes: 0

Related Questions