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.
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.
- We don't want them to share large chunks of copy-and-pasted code since this is error prone.
- Calling -init (or the designated initializer) directly from -initWithCoder could cause problems since it could conflict with the required call to [super initWithCoder: ];
- The only idea I can think of right now is factoring out the common code in to a static function like this:
- (id) init
{
self = [super init];
privateInit(self);
return self;
}
- (id)initWithCoder: (NSCoder*)coder
{
self = [super initWithCoder: coder];
privateInit(self);
if ([coder containsValueForKey: @"foo"]) [self setFoo: [coder decodeObjectForKey: @"foo"]];
if ([coder containsValueForKey: @"bar"]) [self setBar: [coder decodeObjectForKey: @"bar"]];
...
return self;
}
static void privateInit(MyClass *self)
{
self->someCArray = malloc(100);
self->someValue = 123;
...
}
Note that the privateInit function only sets up ivars belonging to the current class, so each superclass would have its own privateInit function to set up the ivars belonging to that superclass.
How does that pattern look?
--Eric
_______________________________________________
Gnustep-dev mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/gnustep-dev