Re: Question on file GSString.m

[Richard Frith-Macdonald]

On 14 Aug 2011, at 22:09, Fred Kiefer wrote:

> On 14.08.2011 22:57, Richard Frith-Macdonald wrote:
>> 
>> On 14 Aug 2011, at 21:20, Fred Kiefer wrote:
>> 
>>> I just noticed that we have lots of reimplementation of NSString
>>> methods in the sub classes in GSString.m. Some of these are
>>> actually needed like implementing
>>> -initWithBytesNoCopy:length:encoding:freeWhenDone:, many others
>>> look plain wrong. Why would we duplicate code here, in some cases
>>> even in a worse form? After the current code freeze on base, could
>>> somebody please go through this file and clean out the duplicated
>>> methods?
>> 
>> Subclasses are *supposed* to reimplement the methods of a class
>> cluster ... 1. the primitive methods have to be implemented for the
>> classes to function at all 2. many/most other methods need to be
>> implemented in terms of the concrete ivars actually used ... so we
>> get decent performance.
>> 
>> There shouldn't be any real duplication of code, but there should be
>> a *lot* of methods re-implemented ...  to access the 8 or 16 bit
>> character data within the strings directly.  These should *not* be
>> 'cleaned out' since they are essential for decent performance.
> 
> There seems to be a disagreement here, so lets look at one example.
> GSPlaceholderString has this method:
> 
> - (id) initWithCharacters: (const unichar*)chars
>                  length: (NSUInteger)length
> {
>  return [self initWithBytes: (const void*)chars
>                     length: length * sizeof(unichar)
>                   encoding: NSUnicodeStringEncoding];
> }
> 
> NSString has the following corresponding method:
> 
> - (id) initWithCharacters: (const unichar*)chars
>                  length: (NSUInteger)length
> {
>  return [self initWithBytes: chars
>                     length: length * sizeof(unichar)
>                   encoding: NSUnicodeStringEncoding];
> }
> 
> I don't quite see how the only difference (the type cast) is "essential for 
> decent performance".

Yes, that does look like a case where we have an unnecessary method override 
... but I had the (possibly mistaken) impression you were talking about a *lot* 
of code and 'worse' code.  It would surprise me if there is much of that.

> I do understand that what you wrote is the basic idea of what should go into 
> this file. I just have the impression that at the moment this isn't what is 
> in the file and I would like to see that corrected. I also know that this has 
> to be done very carefully as just using the implementation in NSString might 
> result in circular call sequences or very bad performance.

Fine ... I agree with that (but don't have time to look at it right now).
IMO the ideal would be if someone could:

1. identify any cases of duplicate/wrong methods
2. write tests for the testsuite to exercise those methods
3. remove the duplication and check that the tests still pass