Review request for java.text.** warning cleanup (original) (raw)
Deepak Bhole dbhole at redhat.com
Mon Dec 5 12:30:50 PST 2011
- Previous message: Review request for java.text.** warning cleanup
- Next message: Review request for java.text.** warning cleanup
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
- John Rose <john.r.rose at oracle.com> [2011-12-02 17:12]:
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.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!)
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)(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
- Previous message: Review request for java.text.** warning cleanup
- Next message: Review request for java.text.** warning cleanup
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]