Review request for java.text.** warning cleanup (original) (raw)

Stuart Marks stuart.marks at oracle.com
Thu Dec 1 18:46:04 PST 2011

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 iterators = new ArrayList(); ++ 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(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/oncodingstyle . 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.)

More information about the jdk8-dev mailing list