Bug 183162

Created attachment 96315 [details]
proposed patch-1

patch with initial impl + tests

Please, review proposed patch

VS1: typo in javadoc "@author Vladiir Voskresensky"
VS2: CharSequences class should be declared final
VS3: missing apichanges.xml entry

(In reply to comment #3)
> VS1: typo in javadoc "@author Vladiir Voskresensky"
Thanks
> VS2: CharSequences class should be declared final
class has private constructor
> VS3: missing apichanges.xml entry
OK

(In reply to comment #4)
> (In reply to comment #3)
> > VS2: CharSequences class should be declared final
> class has private constructor

Yes, but declaring it final does not hurt anything and it's immediately clear to everybody that it really can't be subclassed.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > VS2: CharSequences class should be declared final
> > class has private constructor
> 
> Yes, but declaring it final does not hurt anything and it's immediately clear
> to everybody that it really can't be subclassed.
I don't have real objections. 
Some others can have (Jesse?)... Utility classes with only static methods not often marked with "final", but must have private constructor

Y01 The javadoc is not really finished, is it? Sentences start with lower case, are short, not really describing what is going on, etc.

Y02 Modify apichanges.xml

Y03 Use assertSize. As the primary motivation for this is to save some memory, it is wise to enhanced the tests with assertSize guards.


I guess I'd make the class final as VS02 suggests.

(In reply to comment #7)
> Y01 The javadoc is not really finished, is it? 
semantics of create methods is the same as constructors of String class and has appropriate links.
>Sentences start with lower case,are short, not really describing what is going on, etc.
Will fix
> 
> Y02 Modify apichanges.xml
(VS3) OK
> 
> Y03 Use assertSize. As the primary motivation for this is to save some memory,
> it is wise to enhanced the tests with assertSize guards.
Great. Will use.
> 
> 
> I guess I'd make the class final as VS02 suggests.
OK

Created attachment 96423 [details]
final patch

Please, check patch with addressed comments. If not objections I will integrate tomorrow

Generally I suggest to not mark utility classes final since final and abstract modifiers draw your attention toward subclasses, whereas for utility classes, subclasses are not so much prohibited as meaningless - so a final modifier is just a distraction. Since the ctor is private there is no risk of subclassing anyway. But I do not have a strong opinion.


[JG01] Get rid of ComparatorIgnoreCase. No one is likely to use it, and anyway it is not doing a locale-sensitive comparison like you would expect.


[JG02] CharSequenceKey might be clearer if it were split into two classes, one holding a byte[], one a char[].


[JG03] CharSequencesTest ctor should take a String name and pass it to super. That is how test cases get their names.


[JG04] The test does not appear to ever check non-ASCII character sequences.

(In reply to comment #10)
> Generally I suggest to not mark utility classes final since final and abstract
> modifiers draw your attention toward subclasses, whereas for utility classes,
> subclasses are not so much prohibited as meaningless - so a final modifier is
> just a distraction. Since the ctor is private there is no risk of subclassing
> anyway. But I do not have a strong opinion.
> 
> 
> [JG01] Get rid of ComparatorIgnoreCase. No one is likely to use it, and anyway
> it is not doing a locale-sensitive comparison like you would expect.
Not agree, CND use it.
> 
> 
> [JG02] CharSequenceKey might be clearer if it were split into two classes, one
> holding a byte[], one a char[].
> 
Thanks, good idea. Improve performance.
> 
> [JG03] CharSequencesTest ctor should take a String name and pass it to super.
> That is how test cases get their names.
> 
> 
> [JG04] The test does not appear to ever check non-ASCII character sequences.
Absolutely agree.

(In reply to comment #10)
> Generally I suggest to not mark utility classes final since final and abstract
> modifiers draw your attention toward subclasses, whereas for utility classes,
> subclasses are not so much prohibited as meaningless - so a final modifier is
> just a distraction. Since the ctor is private there is no risk of subclassing
> anyway. But I do not have a strong opinion.
> 
> 
> [JG01] Get rid of ComparatorIgnoreCase. No one is likely to use it, and anyway
> it is not doing a locale-sensitive comparison like you would expect.
We use it (Fortran is case insensitive language and we use comparator to match with known keywords). How to improve it?

> 
> 
> [JG02] CharSequenceKey might be clearer if it were split into two classes, one
> holding a byte[], one a char[].
Will update.

> 
> [JG03] CharSequencesTest ctor should take a String name and pass it to super.
> That is how test cases get their names.
OK

> 
> 
> [JG04] The test does not appear to ever check non-ASCII character sequences.
will add

Thanks.

Created attachment 96545 [details]
next patch

patch with dedicated class for byte-based and char-based impls + updated comparator + tests for Unicode and contentEquals

(In reply to comment #12)
>> [JG01] Get rid of ComparatorIgnoreCase
>
> We use it (Fortran is case insensitive language and we use comparator to match
> with known keywords). How to improve it?

Can this be implemented in the Fortran support code instead? Case-insensitive keyword matching is a relatively uncommon task, since nearly all modern languages are case-sensitive. (Ant is a notable and unfortunate exception, which has led to problems for Turkish users - the infamous I/i/İ/ı problem.)

If it must remain for reasons of encapsulation - i.e. there is no efficient way to implement comparable functionality externally - then make it clear in the Javadoc that this is not a locale-sensitive comparison, thus similar to String.equalsIgnoreCase or .compareToIgnoreCase rather than Collator.

(In reply to comment #14)
> (In reply to comment #12)
> >> [JG01] Get rid of ComparatorIgnoreCase
> >
> > We use it (Fortran is case insensitive language and we use comparator to match
> > with known keywords). How to improve it?
> 
> Can this be implemented in the Fortran support code instead? 
I think, I can move it into Fortran. This comparator currently doesn't use knowledge about optimized classes internal structures (opposite to case sensitive)

> Case-insensitive
> keyword matching is a relatively uncommon task, since nearly all modern
> languages are case-sensitive. 
Do not agree, but off-topic.

re JG01: I would make comparator() method without arguments. Is it ok?

(In reply to comment #16)
> re JG01: I would make comparator() method without arguments.

Yes, that is what I meant.

Thanks for review.
I will integrate.

integrated:
http://hg.netbeans.org/cnd-main/rev/2a8a057f9721

fixed subSequence in changed byte and char long seqs
http://hg.netbeans.org/cnd-main/rev/1d4ace847b93

Integrated into 'main-golden', will be available in build *201004070201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/2a8a057f9721
User: Vladimir Voskresensky <vv159170@netbeans.org>
Log: fixed #183162 -  provide efficient char sequence implementation