On Tue, Apr 26, 2011 at 9:17 PM, Eric Wasylishen <[email protected]>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
> [email protected]
> https://lists.gnu.org/mailman/listinfo/gnustep-dev
>
_______________________________________________
Gnustep-dev mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
