Review request for java.text.** warning cleanup

[John Rose]

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<Integer>)(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<Bar>[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<Attribute> newRunAttributes[] = new Vector<?>[newArraySize];
>> +            @SuppressWarnings("unchecked")  // generic array creation
>> +            Vector<Object> 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<Attribute> newRunAttributes[] = new Vector<?>[ARRAY_SIZE_INCREMENT];
>                                               ^
>  required: Vector<Attribute>[]
>  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<BreakIteratorCache>[] iterCache = new SoftReference[4];
>> ++    @SuppressWarnings("unchecked")
>> ++    private static final SoftReference<BreakIteratorCache>[] 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