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:32:12 UTC 2012

Irony of the day - those changes were done by me! (http://cr.openjdk.java.net/~khazra/7157893/webrev.02/) :D

They were probably a mistake/oversight. I guess the better way is without those extra spaces. See http://docs.oracle.com/javase/tutorial/java/javaOO/annotations.html.

If you have time, maybe you can remove those spaces I put in as a part of this CR.

Thanks! Kurchi

On 8/28/2012 5:23 PM, Dan Xu wrote: > I also thought the space was not needed. But when I made the changes, > I found that many similar codes had the space when two or more warning > types need to be suppressed. For example, java/util/Collections.java, > java/util/Arrays.java, java/util/ComparableTimSort.java, and etc. If > only one warning type needs to be suppressed, I don't see the space in > our codes. Thanks! >> -Dan >>>> On 08/28/2012 05:08 PM, Kurchi Hazra wrote: >> 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. >>>>>> - Kurchi >>>> 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