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

[Kurchi Hazra]

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