macOS Tracking down that memory leak

[larswik]

I had some time tonight and decided to try to track down those 2 memory leaks that has been causing me grief for months now. I first did a find for all words 'alloc' and made sure they had releases, they did. I then questioned my code because I saw an example that you can write over a piece of code with the same name and it was not released and now you have a memory leak.

In the code snippet bellow I am wondering if I can legally do what I did with tempString. In my header I declared an NSString *tempString and I have been using that as a quick place holder. The string is not mutable so it can't change and yet I am changing it. Am I writing over the code causing a memory leak with this style of coding?

- (IBAction)endRoundButton:(id)sender { // *************************   Finish Turn
    
    [COLOR="Red"]tempString = [hpTextField stringValue];[/COLOR]
    if ([tempString isEqualTo:@"0"]) {
        [errorTextField setTextColor:[NSColor redColor]];
        [errorTextField setStringValue:@"NO NEW ENTRY"];
        return;
    }
    [errorTextField setStringValue:@""];
    dieRoll = [goodRollBonusTextField intValue];
    [self convertButton:[hpTextField stringValue]];
    

    critBonus = [critBonusTextField intValue];
    runningCombatTotal =  ((runningCombatTotal + dieRoll) + (hpStorage + critValue + critBonus)); // Adds up the total.
    
    [COLOR="Red"]tempString = [critTextField stringValue];[/COLOR]
    
    if ([tempString isEqualToString:@"-"]) {
        ;
    }
    else{
        tempString = [NSString stringWithFormat:@"%d", hpStorage];
        tempStringTwo = [critTextField stringValue];
        tempString = [tempString stringByAppendingFormat: [tempStringTwo uppercaseString]];
...
..
.
    }

 

[admanimal]

The string is not mutable so it can't change and yet I am changing it. Am I writing over the code causing a memory leak with this style of coding?

You need to make sure that you understand exactly what it means for an NSString or other object to be immutable (a.k.a. not mutable). An NSString is immutable because you can't change the contents of its memory once it is allocated. This is not the same thing as not being able to assign another string (i.e. another piece of memory) to an NSString variable.

You are doing the latter: when you call stringValue or stringWithFormat or stringByAppendingString and assign the result to tempString, you are pointing the tempString variable at a different block of memory with a different string in it. This is different than having tempString point at the same memory the whole time, and you just altering the characters stored in that one block of memory, which is not possible with an immutable string.

Nothing you do with tempString in this method should cause a memory leak, since only autoreleased strings are being assigned to it. A leak could occur if you assign a string that you own to tempString in some other method, and then overwrite that value in this method without releasing it first.

On a related side note, you can't actually assume that any given NSString is really immutable unless you create an immutable copy of it yourself. That's because NSMutableString is a subclass of NSString, so any time you receive an NSString from another method (that you didn't write), it's possible that you are actually being given an NSMutableString that some other class could still alter in the future without you knowing it. That's why the recommended practice is to have NSStrings be copied properties rather than just retained.

 

[itickings]

In my header I declared an NSString *tempString and I have been using that as a quick place holder. The string is not mutable so it can't change and yet I am changing it.

No, you are not. As nicely explained by admanimal, you are never changing any existing string, just making tempString point to different ones.

I would also strongly advise against variables such as tempString, especially global ones. It doesn't make the code easier to read (rather the opposite), and will cause miscellaneous problems sooner or later.

Do you think you are optimizing memory usage, or what is the reasoning behind tempString and friends?

 

[larswik]

My project has a lot of TextFields that need to be converted to Int values to perform math functions. What admanimal wrote is what I thought and I read something like that a few months ago but wanted to confirm it. TempString is exactly what it was intended to do. To hold some random stringValue from a textField as it is being converted to something else.

The odd thing is that when I run the Instruments leak test in Xcode 3 nothing leaks. But in Xcode 4 I have 2. I verified all my allocs had release assigned to them, or autorelease and pool drain. I used no NEW, COPY or RETAIN in the whole project.

As far as I can tell I did not release anything I did not take ownership of. So the only think I can think of is that something got written over with out first being released. or Xcode 4 is wrong and Xcode 3 is right?

 

[admanimal]

TempString is exactly what it was intended to do. To hold some random stringValue from a textField as it is being converted to something else.

What you are describing is exactly a case where you do NOT want to have tempString be an instance variable of the class; it should be local to methods that use it. Generally speaking, variables should always be given the smallest scope possible, because it simplifies debugging. This is a perfect example: if your tempString variable was local to the method you have in your original post, I could tell you for a fact that it was not causing any memory leaks. However, since tempString is (unnecessarily) an instance variable, I can't tell what is happening to it outside of that method, and thus can't say for sure if you are doing anything wrong with it.

 

[itickings]

TempString is exactly what it was intended to do. To hold some random stringValue from a textField as it is being converted to something else.

Trust me, tempString is a horrible name, and re-using the same variable in different situations is a very very bad idea. You may think that it makes sense now, but when you return to the code later it will be much harder to see what a line does.

Compare the following snippet

if ([tempString isEqualTo:@"0"]) {

to

if ([hpString isEqualTo:@"0"]) {

or

if ([[hpTextField stringValue] isEqualTo:@"0"]) {

Notice the difference?

Additionally, you should never ever use a variable with global scope where one with local scope would do.

 

[LostSoul80]

Note that using -[NSString isEqualToString:] is faster than using -[NSObject isEqualTo:].

 

[Sydde]

You should probably ask "how long do I need this value?" Sometimes you can dispense with the variable entirely. For instance, the NSTextField method -stringValue returns a NSString which can be used directly

if ( [[hpTextField stringValue] isEqualToString:@"0"] )

instead of being first stored in a variable. In this particular case, nesting method calls actually does make the code more readable, though it can be taken to extremes (I have taken it to extremes in the past and advise against it).

Using local variables when you need to hold the value only within the method or function is a good idea for another reason. The compiler can see locals, determine how long they need to live and produce code that is equivalent to nested functions (variables are "optimized away"). Thus, you can write code that looks bulky but compiles down to faster, streamlined code. This can be important if there is a problem you cannot figure out going on somewhere in a nested function. Nesting is good if you know you can absolutely rely on the return value to be usable.

 

[larswik]

admanimal - That is a good tip. I will create more local variables instead of global ones. I will get out of the habit of tempString as a default name. I thought it was a good idea and a good label so I know that it was just a temporary item, I guess not.

LostSoul80 - What do you mean by faster, lest steps of computing cycles to get the result?

This is a good project that I really learned a lot about Objective - C. So it is loaded with code that functions but may not be the most well written and clear to readers. Most everything I have written was with the knowledge I had at that time. This is a good time to go back over 900 lines of code and start to simply, clean up and I try and find the memory leaks.

Thanks guys!

 

[LostSoul80]

The documentation states: "isEqualToString: [...] When you know both objects are strings, this method is a faster way to check equality than isEqual:.", and isEqualTo: just calls isEqual:. I've found it helpful when processing lots of comparisons, so if you just call it rarely the performance can be considered equal.

 

[larswik]

Gotcha. Thanks

 

[larswik]

I spent my afternoon trying to find the 2 leaks again. I finally decided to temporarily delete all my methods in the in the implementation section so it was just the @implantation and the @end. It still gave me the same 2 leaks with nothing in there. I got some warnings because stuff was missing but it still opened up in Instruments and found the same 2 leaks.

Can a memory leak come from the variable declaration section in the header file?