Review request for java.text.** warning cleanup
Deepak Bhole
dbhole at redhat.com
Mon Dec 5 12:30:50 PST 2011
* 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
More information about the jdk8-dev
mailing list