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

[Kurchi Subhra Hazra]

Thanks for cleaning up those spaces Dan. The changes look fine.
Sorry for the extra trouble!

- Kurchi

On 8/28/12 10:22 PM, Dan Xu wrote:
> It is funny. :) I have searched all source codes under jdk and removed 
> spaces for the similar cases.
>
> Please review the new version of change at, 
> http://cr.openjdk.java.net/~dxu/7193406/webrev.03/.
>
> Thanks for your comment!
>
> -Dan
>
>
> On 08/28/2012 05:32 PM, Kurchi Hazra wrote:
>> 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
>