Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io (original) (raw)

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Wed Aug 29 00:08:14 UTC 2012

I don't think you need the space before "unchecked" and the one after "rawtypes" in lines 128 and 147 in http://cr.openjdk.java.net/~dxu/7193406/webrev.02/src/share/classes/java/util/PropertyResourceBundle.java.sdiff.html.

On 8/28/2012 4:57 PM, Dan Xu wrote: > Thanks for all your good suggestions! >> I have updated my changes, which revoke changes to makefiles and put > @SuppressWarnings outside methods instead of introducing local > variables for small methods. >> The webrev is at http://cr.openjdk.java.net/~dxu/7193406/webrev.02/. > Thanks! >> -Dan >> On 08/27/2012 04:33 PM, Stuart Marks wrote: >> On 8/27/12 3:55 AM, Doug Lea wrote: >>> The underlying issue is that code size is one of the criteria >>> that JITs use to decide to compile/inline etc. So long as they do >>> so, there will be cases here and there where it critically >>> important to keep sizes small in bottleneck code. Not many, >>> but still enough for me to object to efforts that might >>> blindly increase code size for the sake of warnings cleanup. >>>> I'm pleased that warnings cleanup has attracted this much attention. :-) >>>> I was wondering where rule about @SuppressWarnings and local >> variables originally came from, and I tracked this back to Effective >> Java, Item 24, which says (as part of a fairly long discussion) >>>> If you find yourself using the SuppressWarnings annotation >> on a method or constructor that's more than one line long, >> you may be able to move it onto a local variable declaration. >> You may have to declare a new local variable, but it's worth it. >>>> Aha! So it's all Josh Bloch's fault! :-) >>>> In the warnings cleanup thus far, and in Dan's webrev, we've followed >> this rule fairly strictly. But since we seem to have evidence that >> this change isn't worth it, we should consider relaxing the rule for >> performance-critical code. How about adding a local variable for >> @SuppressWarnings only if the method is, say, longer than ten lines? >> (Or some other suitable threshold.) For short methods the annotation >> should be placed on the method itself. >>>> The risk of suppressing other warnings inadvertently is pretty small >> in a short method, and short methods are the ones most likely to be >> impacted by the addition of a local variable affecting >> compilation/inlining decisions. Right? >>>> (Also, while I've recommended that people follow the local variable >> rule fairly strictly, I think it tends to garbage up short methods. >> So I wouldn't mind seeing the rule relaxed on readability grounds as >> well.) >>>> s'marks >

-Kurchi

More information about the core-libs-dev mailing list