Review request for java.text.** warning cleanup

[John Rose]

On Dec 5, 2011, at 12:30 PM, Deepak Bhole wrote:

>>   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:
>>     [1]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!)
>> 
> 
> Thanks for the pointer! Yes, it is rather unfortunate that it cannot
> changed then :( I have reverted it and added a more localized @SW
> instead.

Good.  I puzzled some more over the localized @SW and found a formulation
that does not require any @SW at all.  It's a matter of choosing wildcards very
carefully.  See patch below.

Basically, it's one of those cases where a type T isn't good enough, but a type
"? extends T" will do the job.  (I can't tell you all such cases, but this is one of them!)

>>   I'm OK with that.  Replacing clone by an equivalent copy constructor
>>   looks trivial enough to me, even though it changes bytecode output.
>> 
> 
> As mentioned in my other email to Rémi, the clone constructor was my
> initial plan but didn't work. Instead I am just initializing a temp var
> (so that I can add a localized @SW) and then assigning the outer scope
> var to it.

Good.

>>   Yep.  SW("serial") seems to be the wrong thing, almost everywhere.
> 
> Out of curiosity, why is this? Is it because not having a serial can
> cause invalid class errors or are there other reasons?

If you don't have a SVUID, the system makes one for you. (It is the same
one your IDE can generate explicitly.)  If you way @SW("serial"), you are
telling the system that it can adjust the SVUID at will.  You get into trouble
with this if somebody adds another (non-transient) field.  This changes the
implicit SVUID.  This can lead to surprising results down the road.

> Thanks for taking a look. New webrev:
> http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.02/
> 
> As with before, diff between rev 01 and 02 is there too:
> http://cr.openjdk.java.net/~dbhole/java.text-warning-cleanup/webrev.02/rev-01-02.patch



On Dec 2, 2011, at 2:12 PM, John Rose wrote:

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

I went back to class AttributeEntry and realized that it also does not need @SW.
Since it is a non-public class, it is OK to change the supertype and the return value
of getKey.  A simple correction to the type parameters of a non-public name is in
scope for this project, as I read Stuart here:
  http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000380.html

-+// Must suppress the whole class to suppress warning about raw supertype Map.Entry.
-+// Parameterizing Map.Entry correctly would requires changing return of getKey.
-+ at SuppressWarnings("rawtypes")
-class AttributeEntry implements Map.Entry {
++class AttributeEntry implements Map.Entry<Attribute,Object> {
...
-    public Object getKey() {
++    public Attribute getKey() {

(I could be mistaken about this; please double check.  I made a public API
mistake in my own java.lang.invoke warnings cleanup!)

Also, I noticed some extraneous blank lines surrounding @SW occurrences.
They aren't needed.  Also, a few occurrences of @SW({"foo"}) do not need
the braces:  @SW("foo").

Here are all the adjustments I suggest, in one patch:
  https://gist.github.com/1440966

See if they work for you.  If so, we're done.

-- John