Review request for java.text.** warning cleanup

[Stuart Marks]

I've filed bug 7117230 to cover this work. I've made John the responsible 
engineer since he's doing the review. John, I hope that's OK.

I have a question about this change to ParseException.java:

+ at SuppressWarnings("serial") // Adding serial ID will break compatibility. 
Suppress it.

Is it really the case that adding the serialVersionUID will break 
compatibility? We've added this to other throwables, e.g. see

http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/624cc18a6cf9

The value declared here is the same as the unmodified version of the class. 
Unless there's something really weird going on with this class that I don't 
see, adding a serialVersionUID should improve compatibility, not break it.

s'marks


On 12/1/11 5:17 PM, John Rose wrote:
> On Dec 1, 2011, at 4:49 PM, John Rose wrote:
>
>> On Dec 1, 2011, at 3:19 PM, Deepak Bhole wrote:
>>
>>> Hi All,
>>>
>>> Here are all the warning fixes for java.text.**:
>>>
>>> http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.00/
>>>
>>> I have added suppressions where fixes would cause compatibility
>>> breakage, or where fixes would be too convoluted (e.g the ever so fun
>>> genericized array creation :)).
>>
>> Thanks for doing all this work!  I have some review comments.
>> ...
>>
>> (About 50% through.  More to come...)
>
> Here's the rest.
>
> I think the method divideUpDictionaryRange is too complex (181 LOC) to be covered by one @SuppressWarnings.  Even adding 30 annotations in the body would be better, since then the other 151 LOC would remain protected.
>
> I do like the informative comment on @SuppressWarnings:
> +    @SuppressWarnings("unchecked") // suppress unchecked warning for cloning stack
>
> Extra parens (only needed when there was a cast) can go away:
> +            cachedBreakPositions[i + 1] = (currentBreakPositions.elementAt(i)).intValue();
> ++            cachedBreakPositions[i + 1] = currentBreakPositions.elementAt(i).intValue();
>
> This one has to be on the method level, of course.  Good comment; suggest s/fallthrough/fallthrough in switch/.
> +    @SuppressWarnings("fallthrough") // fallthrough is expected, suppress it
>
> Diamond operator, again:
> +        ArrayList<AttributedCharacterIterator>  iterators = new ArrayList<AttributedCharacterIterator>();
> ++        ArrayList<AttributedCharacterIterator>  iterators = new ArrayList<>();
>
> and:
> +    private static final Hashtable<Locale, String[]>  cachedLocaleData = new Hashtable<Locale, String[]>(3);
> ++    private static final Hashtable<Locale, String[]>  cachedLocaleData = new Hashtable<>(3);
> +        private static final Map<String, Field>  instanceMap = new HashMap<String, Field>(11);
> ++        private static final Map<String, Field>  instanceMap = new HashMap<>(11);
> +            entryTable = new Vector<EntryPair>(INITIALTABLESIZE);
> ++            entryTable = new Vector<>(INITIALTABLESIZE);
>
>
> This one has to be at class level; good comment:
> + at SuppressWarnings("serial") // Adding serial ID will break compatibility. Suppress it.
>
> There is a peculiar space in the "int []" type name; consider removing it:
> +                          Vector<int []>  eTbl,
> ++                          Vector<int[]>  eTbl,
>
> +    private Vector<int []>  expandTable = null;
> ++    private Vector<int[]>  expandTable = null;
> +            expandTable = new Vector<int []>(INITIALTABLESIZE);
> ++            expandTable = new Vector<int[]>(INITIALTABLESIZE);
> +    private Vector<int []>    expandTable = null;
> (etc.)
>
> Majority usage in that package omits the space, and you delete the minority cases.
>
> Regarding majority usage see M2 in http://blogs.oracle.com/jrose/entry/on_coding_style .
>
> I'm done commenting.
>
> Thanks for taking this on!
>
> Best,
> -- John
>
> P.S.  My recommendations about @SuppressWarnings are off the top of my head.  See Josh Bloch's Effective Java for more and better of that sort of thing.  (Hat tip to Stuart M. and Andrew Hughes.)