Review request for java.text.** warning cleanup (original) (raw)
John Rose john.r.rose at oracle.com
Thu Dec 1 17:17:57 PST 2011
- Previous message: Review request for java.text.** warning cleanup
- Next message: Review request for java.text.** warning cleanup
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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 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.)
- Previous message: Review request for java.text.** warning cleanup
- Next message: Review request for java.text.** warning cleanup
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]