views:

98

answers:

4

Hey guys, another memory management question: I have asked this before, but did not really get an answer:

The question is would the following result in a leak or is it ok?

NSArray *txtArray = [NSArray array];

NSString *aTxtFieldTxt = [[NSString alloc]initWithString:aTxtField.text];
aTxtFieldTxt = [aTxtFieldTxt stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];


NSMutableString *aTxt = [[NSMutableString alloc]initWithString:aTxtFieldTxt];
[aTxtFieldTxt release];


txtArray = [aTxt componentsSeparatedByString:@" "];

aTxt = [[txtArray objectAtIndex:0] retain];


for(int i = 1; i < [txtArray count]; i++){

   [aTxt appendString:@"+"];
   [aTxt appendString:[[txtArray objectAtIndex:i]retain]];
}

This is part of a function. And I am not sure if the assignment of aTxt = [[txtArray objectAtIndex:0] retain]; causes a leak because it is a pointer which originally points to

NSMutableString *aTxt = [[NSMutableString alloc]initWithString:aTxtFieldTxt];
[aTxtFieldTxt release];

How do I do this correctly. Would I have to use another pointer? Can somebody please explain this issue?

Thanks alot!

+1  A: 

Try running your application with Leaks. See if it causes a leak. Leaks is a tool in Instruments.

Nick Brooks
A: 
aTxtFieldTxt = [aTxtFieldTxt stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

This will cause the original reference to aTxtFieldTxt being lost (thus a leak). Even worse, a convenient function will return an object of retain count 0, so the -release later will crash the program.

It's better written as

// don't stuff everything into a single line.
NSCharacterSet* charsToTrim = [NSCharacterSet whitespaceAndNewlineCharacterSet];
// there's no need to create a copy of the string.
NSString* aTxtFieldTxt = [aTxtField.text stringByTrimmingCharactersInSet:charsToTrim];

// don't release stuff with retain count 0.

// you just want to replace all ' ' by '+' right? there's a method for that.
NSString* aTxt = [aTxtFieldTxt stringByReplacingOccurrencesOfString:@" "
                                                         withString:@"+"];

Edit: If you need to replace multiple spaces to one +, you need to parse the string, e.g.

NSCharacterSet* charsToTrim = [NSCharacterSet whitespaceAndNewlineCharacterSet];
NSString* aTxtFieldTxt = [aTxtField.text stringByTrimmingCharactersInSet:charsToTrim];

NSScanner* scanner = [NSScanner scannerWithString:aTxtFieldTxt];
[scanner setCharactersToBeSkipped:nil];
NSMutableString* aTxt = [NSMutableString string];
NSCharacterSet* whitespaceSet = [NSCharacterSet whitespaceCharacterSet];
while (![scanner isAtEnd]) {
   NSString* res;
   [scanner scanUpToCharactersFromSet:whitespaceSet intoString:&res];
   [aTxt appendString:res];
   if ([scanner scanCharactersFromSet:whitespaceSet intoString:NULL])
      [aTxt appendString:@"+"];
}
KennyTM
Hey, thanks alot. So are there ectually two leaks? Because furtherdown is another reassignement of a pointer...?! See my original question...// you just want to replace all ' ' by '+' right? there's a method for that. But what if there are two spaces and I only want one '+'?
jagse
@jagse: (1) No, more than 2 because of many unnecessary `-retain`'s (2) well your original code doesn't show that. `-componentsSeparatedByString:` will split `"5   6"` into `["5","","","6"]`.
KennyTM
Yes right. Its not the whole method, thats why there are retains. But both reassignments cause a leak? Right?
jagse
@jagse: Right .
KennyTM
And if I reassign an autoreleased pointer then there would be no leak?Like mentioned in the answer below: Lots and lots of issues with this code... Would that actually be better because I do not have to create a new pointer and thereby save a tiny little bit of resources?
jagse
@jagse: Yes. Please read the Cocoa memory management guide for detail.
KennyTM
I actually read it before. I think now I really understood all of it. I just never had anything to do with a language where you have to do memory management. Especially the concept of pointers and reassignment was not clear. But it is now. Thanks to you and some other guys here. Thanks!
jagse
+1  A: 

Lots and lots of issues with this code.

//
// Don't do this.  Just declare the txtArray
//
NSArray *txtArray /* = [NSArray array]*/;

//
// You need to auto release after init in this case.
//
NSString *aTxtFieldTxt = [[[NSString alloc]initWithString:[aTxtField text]] autorelease]; 

//
// You are reassigning the aTxtFieldTxt and the new value is returned autoreleased.
//
aTxtFieldTxt = [aTxtFieldTxt stringByTrimmingCharactersInSet:[NSCharacterSet  whitespaceAndNewlineCharacterSet]];

//
// Again, autorelease after init
//
NSMutableString *aTxt = [[[NSMutableString alloc]initWithString:aTxtFieldTxt] autorelease]; 

//
// You never alloced this instance so it needs no release.
//
 /*[aTxtFieldTxt release]; */

//
// This array is returned autoreleased
//
txtArray = [aTxt componentsSeparatedByString:@" "];

//
// No need to retain here.  Just get the object
//
aTxt = /*[*/[txtArray objectAtIndex:0]/* retain]*/;

for(int i = 1; i < [txtArray count]; i++)
{
    [aTxt appendString:@"+"]; 
    [aTxt appendString:[[txtArray objectAtIndex:i]retain]]; 
}

I have found, that if you have retains/releases outside of accessors/base int/dealloc routines, you are doing something wrong. For every alloc you must have a balanced release/retain for that instance of the object. If you reassign the variable, you will loose your reference to it.

This is a quick stab on how I would write this code:

NSArray         *txtArray;
NSString        *aTxtFieldTxt = [NSString stringWithString:[aTxtField text]];
NSMutableString *aTxt;

aTxtFieldTxt = [aTxtFieldTxt stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
aTxt         = [NSMutableString stringWithString:aTxtFieldTxt]; 
txtArray     = [aTxt componentsSeparatedByString:@" "];

aTxt = [NSMutableString stringWithString:[txtArray objectAtIndex:0]];
for(int i = 1; i < [txtArray count]; i++)
{
    [aTxt appendString:@"+"]; 
    [aTxt appendString:[[txtArray objectAtIndex:i]retain]]; 
}
Steven Noyes
So the reassignment of aTxt to [txtArray objectAtIndex:0] does not cause a leak beacuse of the autorelease at the point: NSMutableString *aTxt = [[[NSMutableString alloc]initWithString:aTxtFieldTxt] autorelease]; ???
jagse
Correct. By doing the autorelease when you alloc the NSMutableString (or better just use the stringWithCapacity: method), you can reassign the aTxt and it will go away at some point in the future. The same is true with aTxtFieldTxt.
Steven Noyes
Is it better to reuse the pointer instead of releasing it and creating another one, or does that not matter that much?
jagse
That is the point of autorelease:SomeObject *someobject = [[[SomeObject alloc] init] autorelease];someobject = [SomeObject alloc] init] autorelease];No Leaks.Just reuse the pointer. In general, you should almost never need release/retain outside init/accessors/dealloc if you do things right.
Steven Noyes
A: 

As others pointed out, this code leaks all over the place.

A real good way to find those without even running your code is the static analyzer!

Use Build and Analyze from the Build menu, and it will show you what leaks and why, with complete code paths.

Eiko
Hey, thanks for this tip!
jagse