Re: NSNumberFormatter segfault when using -initWithCoder

[Eric Wasylishen]

Hey Stefan

On 2011-04-26, at 8:45 PM, Stefan Bidi wrote:

On Tue, Apr 26, 2011 at 9:17 PM, Eric Wasylishen <address@hidden> wrote:

Hey.
-[NSNumberFormatter init] has this block of code:

 internal->_behavior = _defaultBehavior;
 internal->_locale = RETAIN([NSLocale currentLocale]);
 internal->_style = NSNumberFormatterNoStyle;
 internal->_symbols = NSZoneCalloc (z, MAX_SYMBOLS, sizeof(id));
 internal->_textAttributes = NSZoneCalloc (z, MAX_TEXTATTRIBUTES, sizeof(id));
 internal->_attributes = NSZoneCalloc (z, MAX_ATTRIBUTES, sizeof(int));

which allocates some C arrays.  However, these ivars are never initialized if you use -initWithCoder:, and so if you call -setDecimalSeparator: on a instance loaded from a nib, you'll get a segfault in this line of code in the SET_SYMBOL macro:

 if (internal->_symbols[key])

since the internal->_symbols will be NULL at that point.

Thanks for spotting that.  I seem to have forgotten about the NSCoding protocol when I added these new ivars.  I see where someone (can't remember if it was me or not) added a FIXME tag in -initWithCoder:.

I have a couple of comments about this:

1. The SET_SYMBOL macro wasted me a significant amount of time because I had to expand it by hand to locate the segfault. Is there a reason this isn't a private instance method instead of a macro? (same with SET_TEXTATTRIBUTE, SET_ATTRIBUTE, SET_BOOL, GET_SYMBOL, GET_ATTRIBUTE, GET_BOOL).
Sorry to sound negative; the new NSNumberFormatter implementation looks really nice otherwise :-)

I don't have a good reason as to why I did it.  I originally used methods but later changed it to macros.

I see. I didn't mean to direct my annoyance at you, but at GDB for not letting you step line by line though macro definitons :-)

I guess it's probably not worth the effort changing these back to methods - the bug wasn't in the macro definitions, anyway.

2. I've seen this mistake (-initWithCoder not doing everything -init does) before, and I expect it occurs elsewhere in GNUstep. Do we have a policy on how to prevent this from happening in the future? i.e. what's the right way to implement -initWithCoder and -init? I think we should have a standard pattern we use everywhere.

I'm OK with this or the private method you mentioned in your other e-mail.  I can't promise I'll get to it right away, though... I haven't had a lot of personal time lately.
 

No worries, I committed a fix already so -initWithCoder: works. (using a private method like I described). I'd be interested to hear what others think about adopting an approach similar to this for other classes that implement NSCoding.

Regards,

Eric