Hi Richard,
On 2011-05-02, at 6:17 AM, Richard Frith-Macdonald wrote:
>
> On 27 Apr 2011, at 03:17, Eric Wasylishen 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:
>
> I rewrote this code to avoid the inefficient and problematic heap memory
> allocation here.
>
>> if (internal->_symbols[key])
>>
>> since the internal->_symbols will be NULL at that point.
>>
>>
>> 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 agree about the readability here ... I rewrote those macros as methods.
>
Great, thanks, it looks a lot better!
>> 2. I've seen this mistake (-initWithCoder not doing everything -init does)
>> before, and I expect it occurs elsewhere in GNUstep.
>
> Possibly ... the problem is somewhat worse than you think though. It doesn't
> just apply to -initWithCoder: as there's a similar issue with classes which
> support -copyWithZone: and -mutableCopyWithZone:
Ah, I forgot about copyWithZone/mutableCopyWithZone.
>
>> 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.
>> - 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?
>
> Not bad at all ... I like the use of a static function for common
> initialisation code (better than a private method) ... I think it's used in a
> few places in gnustep but it's not, in my opinion, a general solution. I
> think the situations are just too variable to support a general/standard
> pattern.
>
> For the vast majority of classes there is little/no special initialisation
> required ... the classes contain simple instance variables whose values
> should either default to zero/nil or be initialized by the decoding methods
> (or by the copying methods) directly, and we should avoid doing extra work.
>
> In the *really* complex cases where you have complicated data structures to
> construct, my preference is to use a private class rather than a lot of
> instance variables in the public class ... in this case the code for -init,
> -initWithCoder:, -copyWithZone: etc will all just call something like 'ivar =
> [PrivateClass new];'
> A good design principle here is lazy initialisation ... complex/expensive
> data structures are set up only at the point there they are actually used ...
> often we don't create them on initialisation of the object.
>
> Between these two extremes we have a other solutions:
>
> Where there is no superclass method which needs calling (very often the
> case), then all the other methods should just call the designated initializer
> (usually -init) ... I think this is the most 'standard' pattern as it is the
> way initialisation is normally done, and it's more intuitive for these odd
> initialisation cases to follow that standard principle.
>
> Where there is superclass initialisation involved, as you point out, we can't
> just call the -init method. In this case I think your solution of using a
> static function or macro to initialise things can be valuable.
>
> Finally, it's sometimes worth using a private method (eg -_init) for instance
> in a class cluster where we actually want subclasses to be able to
> modify/replace parts of the superclass initialisation ... but I'd avoid this
> as a rule.
>
> In summary ... there's no standard pattern to use everywhere, but there are a
> few principles:
> 0. KISS
> 1. design classes where ivars work when they are initialised to zero by
> default, so no extra support is needed.
> 2. don't do complex data structure setup until it's actually needed, and then
> try to use a static function or use a private class to encapsulate the
> complexity.
> 3. keep initialisation in the designated initialiser if possible.
I agree now that it's too complex to have a pattern like the one I proposed
above. Your list of principles sounds really good.
In summary if we find classes with large chunks of complex code copied and
pasted in the designated initializer, copy methods, and -initWithCoder:, some
refactoring is probably needed, following the above principles. Hopefully there
aren't many/any classes like this though :-).
Cheers,
Eric_______________________________________________
Gnustep-dev mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/gnustep-dev
