Review request for java.text.** warning cleanup

[John Rose]

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.)