Re: [PATCH] NumberFormat

[Dalibor Topic]

Hi Guilhem,

Guilhem Lavaux wrote:
> Hi,
>
> Here is just a patch to review about NumberFormat.Field before I check 
> it in. It implements java.text.NumberFormat.Field it is quite 
> straightforward using the documentation.

Looks good to me in general, I have small remarks before you check it in:

> Index: java/text/NumberFormat.java
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/java/text/NumberFormat.java,v
> retrieving revision 1.4
> diff -u -r1.4 NumberFormat.java
> --- java/text/NumberFormat.java 22 Jan 2002 22:27:01 -0000      1.4
> +++ java/text/NumberFormat.java 21 Nov 2003 19:52:44 -0000
> @@ -79,6 +79,98 @@
[snip]
> +    private static final NumberFormat.Field[] allFields =
> +    {
> +      INTEGER, FRACTION, EXPONENT, DECIMAL_SEPARATOR, SIGN,
> +      GROUPING_SEPARATOR, EXPONENT_SYMBOL, PERCENT,
> +      PERMILLE, CURRENCY, EXPONENT_SIGN
> +    };

I don't know what GNU Classpath's policy on JavaDoc comments for private 
 members [1] is, but as Michael Koch is cheering for checkStyle on #gcj 
I assume it will become the patch submission standard, and it pretty 
much wants to see comments on everything, if I recall my last CheckStyle 
usage attempts.

So, could you add a javadoc comment describing the field, please?

> +    // For deserialization purpose
> +    private Field()
> +    {
> +      super("");
> +    }
Same as above.
Could you add a javadoc comment for the constructor, please?

> +    
> +    private Field(String s)
> +    {
> +      super(s);
> +    }

This constructor is protected in the API spec for 1.4.2.

Again, a small comment would be nice.

> +    protected Object readResolve() throws InvalidObjectException
> +    {
> +      String s = getName();
> +      for (int i=0;i<allFields.length;i++)
> +       if (s.equals(allFields[i].getName()))
> +         return allFields[i];
> +
> +      throw new InvalidObjectException("no such NumberFormat field called " + 
> s);
> +    }
> +  }

Looks good to me. could you add another comment here?

Finally, the mean question: do we have mauve tests for those? If not, 
would you write some, please, so that gcj devs don't go ahead again and 
re-implement everything from scratch as they tend to do ;)? [2]

cheers,
dalibor topic,

who is eager to write his own patch to the java/text/FieldPosition.java 
(equals) method's comment as soon as he's finished playing virtual mjw ;)

[1] I devote this pun to ricky clarckson. Mark, how are his 
documentation patches coming along?