Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
dan.xu at oracle.com
Wed Aug 29 00:23:33 UTC 2012
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!
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
> - 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/.
>> 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
More information about the core-libs-dev