Re: [PATCH] Fix NSMenu retainCount problem

[Richard Frith-Macdonald]

On 7 Feb 2004, at 16:28, Quentin Mathé wrote:

> Le 7 févr. 04, à 15:47, Alexander Malmberg a écrit :
>
> > Quentin Mathé wrote:
> >
> > > the user shouldn't have to know the framework implementation. In 
> > > other
> > > terms, correct objective-c memory management needs to support this 
> > > rule
> > > : sending a message to an object should never induce increased retain
> > > count for this object.
> >
> > I don't think this rule should be adopted. It's blatantly false for a
> > huge number of methods.
>
> Well... sure, I would not pretend it's THE rule. :-)
>
> > While I can see that it would be useful in some
> > odd cases, the huge number of exceptions would make it difficult to 
> > rely
> > on. Cascading exceptions would make things even worse. For example, 
> > note
> > that methods implemented like:
> >
> > -(NSString *) uppercaseString
> > {
> >         if (!self_contains_lowercase_characters)
> >                 return [[self copy] autorelease];
> >         else
> >         {
> >                 /* create new string, etc. */
> >         }
> > }
> >
> > violate this rule and would have to rewritten in a less clear and less
> > efficient way.
>
> I don't understand why this method violates this rule, because the 
> retain count for self isn't modified, the method just returns a copy 
> or a new instance and I said nothing about what the method can 
> return... perhaps there is something screwed up in my thinking but I'm 
> not seeing it.

Many classes (constant strings for example) implement copy as retain ...
so the above method would retain and autorelease self.

> > The library should be free to autorelease objects. Library methods
> > should use their own autorelease pools if they autorelease a "large"
> > number of objects, and must always use one if the number of 
> > autoreleased
> > objects is unbounded. However, in the end, when the method returns, 
> > the
> > number of autoreleases on the objects that have been involved is
> > unspecified.
>
> No problem with that. I'm just saying if you write :
>
> [self retain];
> blabla
> [self autorelease];
>
> you must use a local autorelease pool and release it at the end of the 
> method.

There is no such constraint though ... many methods/classes will do
a retain and autorelease of self without wrapping it in a local 
autorelease
pool, and there is really no reason why they should not chose to do so.

> > If you're relying on exactly when -dealloc is called for an instance
> > (which should be very rare), you need to explicitly manage autorelease
> > pools to ensure that there are no pending autoreleases. Alternatively,
> > you can split out the necessary "invalidation" from the -dealloc (like
> > eg. -base's NSConnection).
>
> Well I see your point... but for NSMenu, with this approach, I must 
> write :
>
> - (void)methodForContextMenuExample
> {
>   NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>   NSMenu *menu = [[NSMenu alloc] initWithTitle:@"bip"];
>
>   [menu addItem:menuItem];
>
>  [menu setMenuChangedMessagesEnabled:YES]; // to process the 
> notifications still around
>  [pool release]; // to release the notifications which retain the menu
>
>   [menu release]; // now we are sure the menu will be deallocated
>
>   // Now I can reuse menu Item
>   menu = [[NSMenu alloc] initWitTitle:@"hop"];
>   [menu addItem:menuItem];
> }
>
> Not very obvious... how will you explain to other developers a such 
> pratice ? Specific documentation for each GNUstep classes ?

No ... I think the above code is wrong for the same reason that your 
original code its wrong ...
it depends upon an undocumented, implementation specific feature.

> With Cocoa, to do the same thing, I just need to write :
>
> - (void)methodForContextMenuExample
> {
>   NSMenu *menu = [[NSMenu alloc] initWithTitle:@"bip"];
>
>   [menu addItem:menuItem];
>
>   [menu release]; // we are sure the menu will be deallocated
>
>   // Now I can reuse menu Item
>   menu = [[NSMenu alloc] initWitTitle:@"hop"];
>   [menu addItem:menuItem];
> }
>
> I prefer that, more obvious and clean...

But depending on an undocumented feature of the MacOS-X implementation  
of NSMenu ...
you are expecting the menu to be destroyed when you release it, and 
that might not be so.
The code may not work in the next release... it may even (depending on 
how NSMenu is
actually implemented) not work sometimes in your own application ... 
since you don't know
under what circumstances the menu is actually deallocated ... perhaps 
the system may
decide to store it in a cache for some time under some circumstances.

Isn't the 'correct' code to move the item from one menu to the other 
like this ....

- (void)methodForContextMenuExample
{
  NSMenu *menu = [[NSMenu alloc] initWithTitle:@"bip"];

  [menu addItem:menuItem];
  [menu removeItem:menuItem];
  [menu release];
  menu = [[NSMenu alloc] initWitTitle:@"hop"];
  [menu addItem:menuItem];
}


All that being said ... I like GNUstep implementation details to be as 
close to MacOS-X
as we can reasonably manage ... so if NSMenu *seems* to never retain 
itsself, it would
be nice if the GNUstep implementation did the same, but that's not a 
reason to invent a
rule about memory management which does not exist in MacOS-X, nor is it 
a reason to
write user code which depends on undocumented features of the system... 
it's just a
user friendly practice to cope as best we can when user code does make 
bad assumptions.