Review request for java.text.** warning cleanup

[Deepak Bhole]

* Stuart Marks <stuart.marks at oracle.com> [2011-12-14 18:21]:
> 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.
> 

Hi Stuart,

As Andrew already mentioned, I was away and got back on Friday. I've
just sent a message to John to get the final matters sorted out.
Hopefully this will get pushed soon!

Cheers,
Deepak

> 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