Re: NSNotificationCenter's pointer abuse

[Richard Frith-Macdonald]

On 8 May 2013, at 23:14, Fred Kiefer <address@hidden> wrote:

> On 08.05.2013 13:34, David Chisnall wrote:
>> Compiling NSNotificationCenter with the latest clang gives me these errors:
>> 
>> In file included from NSNotificationCenter.m:262:
>> ../Headers/GNUstepBase/GSIMap.h:466:3: warning: bitmasking for introspection 
>> of Objective-C object pointers is
>>       strongly discouraged [-Wdeprecated-objc-pointer-introspection]
>>   GSI_MAP_RELEASE_KEY(map, node->key);
>>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> NSNotificationCenter.m:243:61: note: expanded from macro 
>> 'GSI_MAP_RELEASE_KEY'
>> #define GSI_MAP_RELEASE_KEY(M, X) ({if ((((uintptr_t)X.obj) & 1) == 0) \
>>                                          ~~~~~~~~~~~~~~~~~~ ^
>> In file included from NSNotificationCenter.m:262:
>> ../Headers/GNUstepBase/GSIMap.h:1183:8: warning: bitmasking for 
>> introspection of Objective-C object pointers is
>>       strongly discouraged [-Wdeprecated-objc-pointer-introspection]
>>               GSI_MAP_RELEASE_KEY(map, node->key);
>>               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> NSNotificationCenter.m:243:61: note: expanded from macro 
>> 'GSI_MAP_RELEASE_KEY'
>> #define GSI_MAP_RELEASE_KEY(M, X) ({if ((((uintptr_t)X.obj) & 1) == 0) \
>>                                          ~~~~~~~~~~~~~~~~~~ ^
>> 
>> 
>> The error exists because in recent runtimes (GNUstep and Apple), some small 
>> objects are stored inside pointers and so you should not set or inspect the 
>> low bits unless you really know what you're doing.  This code, however, 
>> predates that and the assumption wasn't caught up until now.
>> 
>> The low bit is used for two purposes: to 'hide' some pointers from GC (I'm 
>> not sure why this needs to be done, as we should be using zeroing weak 
>> references for that, and do appear to be doing so, and, perhaps more 
>> importantly, we don't want objects to be deallocated but leave dangling 
>> pointers in GC mode), and to differentiate between keys and objects.

I'm fairly sure this code dates from before we had zeroing weak pointers in 
Boehm GC.   It should be possible to safety change it now.

>> The latter is problematic, because we will potentially have cases where this 
>> differentiation is flawed (in particular, if a notification has a very short 
>> string name).  It's not totally clear to me that this ought to be needed, 
>> because it seems that we store them in different map tables.  Is this just a 
>> limitation of GSIMap (only allowing one map table type per compilation 
>> unit)?  In which case we could remove it by using NSMapTable.
> 
> If GSIMap wont let us mix different usages in on file

That's a big IF...
and of course it's not true ... the macro is passed the map as an argument and 
is therefore able to handle different maps in different ways.
NSMapTable is implemented on top of GSIMap, so switching to it would just make 
the code less efficient ... it makes more sense to just change the macros to 
handle the new objects produced by the new objc runtime.