Review request for java.text.** warning cleanup

[Deepak Bhole]

* John Rose <john.r.rose at oracle.com> [2011-12-02 17:12]:
>    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:
>      [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.

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

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

Done!

[ ... ]
> 
>    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.)
> 

I've added the deprecated method names in a comment. I avoided adding
the body just in case it changes by the time someone gets to 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.
> 

Out of curiosity, why is this? Is it because not having a serial can
cause invalid class errors or are there other reasons?

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

Cheers,
Deepak