Re: problems compiling NSAnimation.m

[Richard Frith-Macdonald]

On 10 May 2007, at 14:57, Xavier Glattard wrote:

> Richard Frith-Macdonald <richard <at> tiptree.demon.co.uk> writes:
>
> > On 10 May 2007, at 12:35, Nicola Pero wrote:
> (...)
> > > This looks pretty bad, not only because it doesn't compile with GCC
> > > 2.95, but also because that variable defined in the middle of
> > > nowhere is very ugly ... unclear
> > > scope (what happens if you have two _NSANIMATION_LOCK in
> > > sequence ?  is the same variable being used or different
> > > variables ?) ...
>
> This macro is used as a macro : to repeat again and again the same
> piece of code. It only has to be used in this class. If you have two
> _NSANIMATION_LOCK then you have made an error : would you code two
> [NSThread -lock] in sequence ?

You might ... if one method locks  then calls another method which  
also locks.  Of course, if this is possible then the lock needs to be  
a recursive one.

> > I fixed this to make __gs_isLocked an ivar rather than declaring it
> > locally (which was pretty suboptimal).  I also fixed a bug in the
> > unlock macro (it was setting the lag to the wrong value), and added
> > assertions to check that the macro is not misused.
>
> Bad. If __gs_locked is needed (and as i said i think it is not : i  
> made
> an error) then it must be declared in all methods. If __gs_isLocked is
> an ivar, its value can be changed in another method then the [unlock]
> will never be called.

Sure, but similarly if it's a local variable its value can be  
incorrectly changed within a method or function where it is declared.

> An ivar already exists: _isThreaded.

But that doesn't record the same thing.  The __gs_locked boolean  
records whether the lock is locked, not whether the animation is  
threaded.

However, quite likely you are correct and it's not needed and the  
whole thing can be removed ... I only looked at the code enough to  
fix it to compile and manage locking correctly, not enough to  
optimise/restructure.  If/when this code is restructured, it would  
IMO be good to replace all the ivars with a single pointer to a  
private structure so that future changes to ivar layout can be done  
without breaking binary compatibility between releases.