8

I'm still trying to understand this piece of code that I found in a project I'm working on where the guy that created it left the company before I could ask.

This is the code:

-(void)releaseMySelf{
    for (int i=myRetainCount; i>1; i--) {
        [self release];
    }
    [self autorelease];
}

As far as I know, in Objective-C memory management model, the first rule is that the object that allocates another object, is also responsible to release it in the future. That's the reason I don't understand the meaning of this code. Is there is any meaning?

Cœur
  • 37,241
  • 25
  • 195
  • 267
Ricardo
  • 2,831
  • 4
  • 29
  • 42
  • 20
    No surprise he left the company; code like that is a sure sign that he was in way over his head, had no understanding of how to write an app and no opportunity to learn the proper ways. If I found that in a project (and I have -- used run a consulting services company that did "clean up" on troubled projects), I would immediately throw a red flag and demand that every line of code written by that person be reviewed. (It also looks like he is maintaining a retain count separate from the object's actual retain count... 2 red flags on that play.) – bbum Dec 16 '11 at 17:13
  • 9
    I think "I want to know who wrote that so I can be sure to never work with him" – Dave DeLong Dec 16 '11 at 17:22
  • 1
    Oy vey. I just threw up in my mouth. – Mike C. Dec 16 '11 at 18:13
  • 1
    Where does myRetainCount come from? Where is it defined? Where else is it used? Without that, it's tough to judge whether the code might not be OK after all. Well, maybe not OK, but at least not buggy, just bad style. – uliwitness Dec 16 '11 at 18:41
  • 1
    @uliwitness Um, no. No matter what `myRetainCount` does, this is not OK. – Jonathan Sterling Dec 16 '11 at 18:50
  • 1
    @uliwitness If he were implementing his own reference counting system on top of the system's RC, then that could be valid code, though it would also require some kind of a solution like zeroing-weak-on-dealloc to make it correct. At best, a bad re-invention of wheels. – bbum Dec 16 '11 at 22:48
  • The funniest code I've ever read. LOL :)) – hiepnd Dec 19 '11 at 02:11

4 Answers4

17

The author is trying to work around not understand memory management. He assumes that an object has a retain count that is increased by each retain and so tries to decrease it by calling that number of releases. Probably he has not implemented the "is also responsible to release it in the future." part of your understanding.

However see many answers here e.g. here and here and here.

Read Apple's memory management concepts.

The first link includes a quote from Apple

The retainCount method does not account for any pending autorelease messages sent to the receiver.

Important: This method is typically of no value in debugging memory management issues. Because any number of framework objects may have retained an object in order to hold references to it, while at the same time autorelease pools may be holding any number of deferred releases on an object, it is very unlikely that you can get useful information from this method. To understand the fundamental rules of memory management that you must abide by, read “Memory Management Rules”. To diagnose memory management problems, use a suitable tool: The LLVM/Clang Static analyzer can typically find memory management problems even before you run your program. The Object Alloc instrument in the Instruments application (see Instruments User Guide) can track object allocation and destruction. Shark (see Shark User Guide) also profiles memory allocations (amongst numerous other aspects of your program).

Community
  • 1
  • 1
mmmmmm
  • 32,227
  • 27
  • 88
  • 117
  • 3
    On a side note, I recommend you removing this piece of code and running your code through static analyzer. It should show where objects should've been released. – Eimantas Dec 16 '11 at 11:30
7

Since all answers seem to misread myRetainCount as [self retainCount], let me offer a reason why this code could have been written: It could be that this code is somehow spawning threads or otherwise having clients register with it, and that myRetainCount is effectively the number of those clients, kept separately from the actual OS retain count. However, each of the clients might get its own ObjC-style retain as well.

So this function might be called in a case where a request is aborted, and could just dispose of all the clients at once, and afterwards perform all the releases. It's not a good design, but if that's how the code works, (and you didn't leave out an int myRetainCount = [self retainCount], or overrides of retain/release) at least it's not necessarily buggy.

It is, however, very likely a bad distribution of responsibilities or a kludgey and hackneyed attempt at avoiding retain circles without really improving anything.

uliwitness
  • 8,532
  • 36
  • 58
  • However it is still not the correct way to implement - it is still the author not getting memory management correct - retain/release etc works with threads – mmmmmm Dec 17 '11 at 00:06
  • Threads were just meant as an example of clients that one would typically register/unregister, e.g. for thread pools or "cancel all" behaviour. – uliwitness Dec 18 '11 at 20:07
3

This is a dirty hack to force a memory release: if the rest of your program is written correctly, you never need to do anything like this. Normally, your retains and releases are in balance, so you never need to look at the retain count. What this piece of code says is "I don't know who retained me and forgot to release, I just want my memory to get released; I don't care that the others references would be dangling from now on". This is not going to compile with ARC (oddly enough, switching to ARC may just fix the error the author was trying to work around).

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
2

The meaning of the code is to force the object to deallocate right now, no matter what the future consequences may be. (And there will be consequences!)

The code is fatally flawed because it doesn't account for the fact that someone else actually "owns" that object. In other words, something "alloced" that object, and any number of other things may have "retained" that object (maybe a data structure like NSArray, maybe an autorelease pool, maybe some code on the stackframe that just does a "retain"); all those things share ownership in this object. If the object commits suicide (which is what releaseMySelf does), these "owners" suddenly point to bad memory, and this will lead to unexpected behavior.

Hopefully code written like this will just crash. Perhaps the original author avoided these crashes by leaking memory elsewhere.

mtrent
  • 21
  • 1