Re: [Gap-dev-discuss] Cythiune : fix badly written macros causing nasty crashes

[Philippe Roussel]

Hi,

Le 17/05/2012 14:51, Riccardo Mottola a écrit :
> Hi,
> 
> I think there is no problem releasing a nil object (small speed penalty
> which, seen  the size of Cynthiune is unimportant.
> 
> I don't like these macros, but I don0't see whileyou rewrite them in
> such a funny way.
> 
> Stuff like that is written in base is:
> 
> #define TEST_RELEASE(object)    ({\
> id __object = (object); if (__object != nil) [__object release]; })
> #endif
> 
> #define ASSIGN(object,value)    ({\
>   id __object = object; \
>   object = [(value) retain]; \
>   [__object release]; \
> })
> #endif
> 
> 
> I think ASSIGN should be used instead of SET, RELEASEIFSET should be
> removed instead.
> 
> I will remove all RELEASEIFSET instances for now, think about the former.

Ok, I'm all in favour of using ASSIGN and RELEASE but the SET macro has
to be fixed. Look at this code :

if (cComment)
   SET (*string, [NSString stringWithUTF8String: cComment]);

With the current code it is translated to :

if (cComment)
  if (X) [X release]; X = Y; if (X) [X retain];

which you can write :

if (cComment)
  if (X)
    [X release];
X = Y;
if (X)
  [X retain];

You see that that last part is executed event if cComment is NULL but it
isn't the intention of the code. So you can use the way it's written in
base (using '({') or my way but the actual code is wrong.

And fixing the macros is just easier that auditing the code and does
less churn than going for ASSIGN and RELEASE.

Anyway, I'm fine with any solution that fixes the code.

Thanks

> Riccardo
> Philippe Roussel wrote:
>> Hi,
>>
>> The ChangeLog entry says it all.
>>
>> Philippe
>>
>> Index: ChangeLog
>> ===================================================================
>> RCS file: /sources/gap/gap/user-apps/Cynthiune/ChangeLog,v
>> retrieving revision 1.46
>> diff -u -r1.46 ChangeLog
>> --- ChangeLog    16 May 2012 16:28:33 -0000    1.46
>> +++ ChangeLog    17 May 2012 11:09:34 -0000
>> @@ -1,3 +1,13 @@
>> +2012-05-17  Philippe Roussel<address@hidden>
>> +
>> +    * Frameworks/Cynthiune/utils.h
>> +    Fix badly written macros SET and RELEASEIFSET (which could be
>> replaced
>> +    by GNUstep's ASSIGN and RELEASE) to not have side effects.
>> +    The following code in VorbisTags.m
>> +      if (cComment)
>> +            SET (*string, [NSString stringWithUTF8String: cComment]);
>> +    wasn't functioning as planned because of the missing curly brackets
>> +
>>   2012-05-15 Riccardo Mottola<address@hidden>
>>
>>       * PlaylistController.m
>> Index: Frameworks/Cynthiune/utils.h
>> ===================================================================
>> RCS file:
>> /sources/gap/gap/user-apps/Cynthiune/Frameworks/Cynthiune/utils.h,v
>> retrieving revision 1.2
>> diff -u -r1.2 utils.h
>> --- Frameworks/Cynthiune/utils.h    30 Apr 2012 15:54:30 -0000    1.2
>> +++ Frameworks/Cynthiune/utils.h    17 May 2012 11:09:34 -0000
>> @@ -30,8 +30,17 @@
>>
>>   @class NSArray;
>>
>> -#define SET(X,Y) if (X) [X release]; X = Y; if (X) [X retain]
>> -#define RELEASEIFSET(X) if (X) [X release]
>> +#define SET(X,Y) do { \
>> +  if (X) \
>> +    [X release]; \
>> +  X = Y; \
>> +  if (X) \
>> +    [X retain]; \
>> +} while (0)
>> +#define RELEASEIFSET(X) do { \
>> +  if (X) \
>> +    [X release]; \
>> +} while (0)
>>   #define RETURNSTRING(X) return ((X) ? [NSString stringWithString: X]
>> : @"")
>>
>>   #ifdef __MACOSX__
>>
>