Review request for java.text.** warning cleanup

[Stuart Marks]

Hi Deepak,

Any update on this? It sounds like it's fairly close to being finished. Just 
because Warnings Cleanup Day (Week) is over doesn't mean that we should give up.

s'marks

On 12/6/11 5:26 PM, John Rose wrote:
> 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