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

John Rose john.r.rose at oracle.com
Fri Dec 2 14:12:04 PST 2011

On Dec 2, 2011, at 10:47 AM, Deepak Bhole wrote:

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

Since this is an important point, I checked into it. Result: Wildcards cannot (in general) be compatibly switched with named type parameters in public APIs.

See the demonstration here: https://gist.github.com/1424776

(This is subject to correction by someone more expert than me in API evolution, like Alex Buckley, Joe Darcy, or Maurizio Cimadamore. I'm a JVM hacker!)

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.

I'm OK with that. Replacing clone by an equivalent copy constructor looks trivial enough to me, even though it changes bytecode output.

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.

I didn't get that clearly from the comment. Suggest: +// Suppress warning about raw Map.Entry. Parameterizing Map.Entry correctly +// requires changing return of getKey ++// Must suppress the whole class to suppress warning about raw supertype Map.Entry. ++// Parameterizing Map.Entry correctly would requires changing return of getKey.

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<?>[]

Sorry, I was not explicit. You need a cast. Here's a complete account: https://gist.github.com/1425032

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.

My bad! See 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.

Right. Don't inline the deprecated method body. Just suppress the deprecation warning, with a comment. (Maybe put the body expressions in a comment, for the next person who touches the code.)

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

OK. You could also have put on a cast plus @SW("unchecked").

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.

Yep. SW("serial") seems to be the wrong thing, almost 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!

It looks good to me, after you get rid of SW(rawtypes) and remove (sadly!) the named type parameters.

-- John

More information about the jdk8-dev mailing list