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

Rémi Forax forax at univ-mlv.fr
Fri Dec 2 11:20:45 PST 2011

Hi Deepak,

On 12/02/2011 07:47 PM, Deepak Bhole wrote:

* John Rose<john.r.rose at oracle.com> [2011-12-01 19:49]:

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. Bottom line: In some few cases, I think the the @SuppressWarnings annotations can be refined. Also, I am worried that there are one or two API signature changes or behavior changes. Details: On chunks like this: - HashSet keys = new HashSet(); + HashSet keys = new HashSet(); consider using new HashSet<>(). See also occurrences of new Vector, new HashSet<Map.Entry<Attribute, Object>>, SoftReference, etc. Ah, I wasn't sure if I should have used 7-specific features so I stayed away from them (when bootstrapping when ecj, we found diamond to be problematic as older ecj versions cannot understand it). I will change these then.

You can use them everywhere but in langtools because the compiler need to bootstrap itself.

(Netbeans can find opportunities for diamond operators, BTW.)

The new parameters<K extends Attribute, V> are clearly more correct, but they look like an API change on a public method or constructor. If so, I think you'll have to find a different approach. (But I'm not an API change and generics expert.) Do you mean the changes in the AttributedString constructor? I just added names for the bounded and unbound wildcards so that they could be used in the body (without it, the compiler was throwing errors). I am not expert on these either -- I didn't know adding names could change signature in an incompatible way. I just wrote a small test case to try this out and everything seems to work fine. Adding named parameters does not appear to break compatibility in my test case. A trivial refactoring to use a private parameterized method might work nicely, but we're not doing refactorings today. Yeah, I wanted to stay away from anything that added/removed any code :) For createRunAttributeDataVectors, ensureRunBreak, etc. consider pushing the @SuppressWarnings down onto the individual declarations. It is best to put @SuppressWarnings on the smallest program unit possible, and a local variable declaration is often the right spot. I see that createRunAttributeDataVectors could be viewed as simple enough to mark as a whole, but ensureRunBreak is probably too complex to mark as a whole. Doh! Good point. For DictionaryBasedBreakIterator.divideUpDictionaryRange, I cannot add it above the "bestBreakPositions = (Stack)(currentBreakPositions.clone());" line without refactoring. I done done a very minor refactor (use copy constructor instead of clone) -- please let me know if it is satisfactory.

It seems you still use clone, you can't replace a call to clone with a call to a copy constructor because if Stack is subclassed, clone will work but not the copy constructor.

As for others, I've narrowed it in the new webrev. Narrowing also exposed a few more places I had missed in ensureRunBreak. Fixed now.

In the same way, consider pushing the annotation into the body of class AttributeEntry, even though that class is very simple. That one is for the declaration itself -- the warning is about the Map supertype not being parameterized. For generic array creation, I suggest that you get rid of the "rawtypes" warning on the array element type by adding, before suppressing the "unchecked" warning. Basically, when you find yourself writing new Foo[len], change it to new Foo[len]. Then, if you still get an unchecked warning, add SuppressWarnings.

- Vector newRunAttributes[] = new Vector[newArraySize]; - Vector newRunAttributeValues[] = new Vector[newArraySize]; + @SuppressWarnings("unchecked") // generic array creation + Vector newRunAttributes[] = new Vector<?>[newArraySize]; + @SuppressWarnings("unchecked") // generic array creation + Vector newRunAttributeValues[] = new Vector<?>[newArraySize]; I tried that yesterday, but the compiler threw errors: ../../../src/share/classes/java/text/AttributedString.java:420: error: incompatible types Vector newRunAttributes[] = new Vector<?>[ARRAYSIZEINCREMENT]; ^ required: Vector[] found: Vector<?>[]

Yes, you need to add a cast,

Vector[] newRunAttributes = (Vector[])new Vector<?>[ARRAY_SIZE_INCREMENT];

but if Java is reified in 1.9 as state the current plan, we will be in trouble.

It is probably a good idea to put a comment on most @SuppressWarnings annotations.

Similarly: - private static final SoftReference[] iterCache = new SoftReference[4]; + + @SuppressWarnings({"unchecked","rawtypes"}) + private static final SoftReference[] iterCache = new SoftReference[4]; ++ @SuppressWarnings("unchecked") ++ private static final SoftReference[] iterCache = new SoftReference<?>[4]; Similar error as above. There is an apparent behavior change in CollationElementIterator.setOffset, in this chunk: - int c = text.setIndex(newOffset); + text.setIndexOnly(newOffset); + int c = text.current(); setIndex is deprecated, so I looked into what the method did and did it at the call site. Sorry, I should have specifically mentioned this re-factor in the original email :( I am fine with just suppressing deprecation warning there. I have done so in the new copy by adding the annotation to the method. (About 50% through. More to come...) (from the second email): 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 Fixed with a minor re-factor as mentioned above. 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(); Oops. Fixed in all locations (there were 3 others). 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 Done! 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); Fixed all updates in the patch to use diamond. This one has to be at class level; good comment: + at SuppressWarnings("serial") // Adding serial ID will break compatibility. Suppress it. I saw Stuart's comments on this. Didn't know that there was a way to find existing serialid (I thought Eclipse just generated a random one)! I have added the class default id. 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; Ah, I copied what was there before in RBTableBuilder: int[] valueList = (int [])expandTable.elementAt(i); Fixed everywhere. New webrev: http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.01/ For convenience, I've also uploaded a diff between previous webrev and new one to make it clear as to what changed: http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.01/rev-00-01.patch Thanks for the quick and detailed review! Cheers, Deepak

cheers, Rémi

More information about the jdk8-dev mailing list