Re: [PATCH] RuleBasedCollator

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Guilhem" == Guilhem Lavaux <guilhem@kaffe.org> writes:

Guilhem> Please review.

I read through this.  The code looks fine to me, based on what I
remember of RuleBasedCollator.  I'm glad to see that someone is paying
attention to java.text -- this is some of our oldest and
least-worked-on code, it really deserves a face-lift.


There were a few formatting issues: lines past 79 columns that should
be wrapped, and lack of whitespace around some operators, eg:

Guilhem>     for (i=0;i<prefix.length() && i < s.length();i++)

You need spaces around "=", "<", and after ";" here.


I also had a couple other questions.

Guilhem>   class CollationElement

You changed this from a final class, but I didn't see anything derived
from it.  Any reason why?  I find marking helpers like this "final"
nice because it is a way to say "don't worry about subclasses if you
change this".

Guilhem> 	// Hehehe. What we would do not to use if.
Guilhem> 	try
Guilhem> 	  {
Guilhem> 	    ord1 = ord1block.getValue();
Guilhem> 	  }
Guilhem> 	catch (NullPointerException _)
Guilhem> 	  {
Guilhem> 	    if (ord2block == null)
Guilhem> 	      return 0;
Guilhem> 	    return -1;
Guilhem> 	  }

I'd prefer a simple "if" as I find it clearer to read.

Tom