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

[Rémi Forax]

On 08/30/2012 01:39 AM, Dan Xu wrote:
>
> On 08/29/2012 12:11 PM, Dan Xu wrote:
>>
>> On 08/29/2012 08:27 AM, Joe Darcy wrote:
>>> Hello,
>>>
>>> On 8/29/2012 1:48 AM, Rémi Forax wrote:
>>>> On 08/29/2012 08:33 AM, Kurchi Subhra Hazra wrote:
>>>>> 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
>>>>
>>>> Hi Dan,
>>>>
>>>> In PreHashedMap, the code should be
>>>>
>>>>   Map.Entry<?,?> that = (Map.Entry<?,?>)ob;
>>>>
>>>> so the @SuppressWarnings is not needed anymore.
>>>>
>>>> And in java.util.Arrays, asList() should not be annotated with 
>>>> @SuppressWarnings because it is already annotated with SafeVarargs, 
>>>> so the compiler should not report the varargs warning.
>>>>
>>>> I have CC to compiler-dev to be sure that this is a compiler error.
>>>>
>>>> cheers,
>>>> Rémi
>>>>
>>>
>>> I've spoken to Stuart about this last point.  The @SafeVarargs 
>>> covers uses of Array.asList and covers the declaration of the asList 
>>> method itself.  However, it does not cover calls to the constructor 
>>> inside the asList method, a constructor which itself is *not* 
>>> @SafeVarargs.  Solutions include
>>>
>>> * @SuppressWarnings("varargs") on the asList method
>>> * @SuppressWarnings("varargs")  on new temporary variable for that 
>>> purpose (with possible class file size consequences)
>>> * @SafeVarargs on the ArrayList constructor
>>>
>>> Regards,
>>>
>>> -Joe
>> The first change to the PreHashedMap is right, and I will update my fix.
>>
>> For the second comment, as Joe mentioned, it is necessary. Otherwise, 
>> the compiler will give the following warnings.
>>
>>    ../../../src/share/classes/java/util/Arrays.java:2837: warning:
>>    [varargs] Varargs method could cause heap pollution from
>>    non-reifiable varargs parameter a
>>             return new ArrayList<>(a);
>>                                    ^
>>    ../../../src/share/classes/java/util/Arrays.java:2837: warning:
>>    [varargs] Varargs method could cause heap pollution from
>>    non-reifiable varargs parameter a
>>             return new ArrayList<>(a);
>>
>>                                ^
>>
>> The first and second options works fine. But if I follow the third 
>> option to add @SafeVarargs on the ArrayList constructor, I get the 
>> following error.
>>
>>    ../../../src/share/classes/java/util/Arrays.java:2849: error:
>>    Invalid SafeVarargs annotation. Method ArrayList(E[]) is not a
>>    varargs method.
>>             ArrayList(E[] array) {
>>             ^
>>       where E is a type-variable:
>>         E extends Object declared in class ArrayList
>>
>>
>> So I tried further to change the constructor to a varargs method and 
>> add the @SafeVarargs annotation, but the compiler still gives me 
>> warnings like this,
>>
>>    ../../../src/share/classes/java/util/Arrays.java:2837: warning:
>>    [varargs] Varargs method could cause heap pollution from
>>    non-reifiable varargs parameter a
>>             return new ArrayList<>(a);
>>                                    ^
>>    ../../../src/share/classes/java/util/Arrays.java:2837: warning:
>>    [varargs] Varargs method could cause heap pollution from
>>    non-reifiable varargs parameter a
>>             return new ArrayList<>(a);
>>                                    ^
>>    ../../../src/share/classes/java/util/Arrays.java:2852: warning:
>>    [varargs] Varargs method could cause heap pollution from
>>    non-reifiable varargs parameter array
>>                 a = array;
>>                     ^
>>    3 warnings
>>
>>
>>
>> I think the current change is good for this one. Thanks for your 
>> comments.
>>
>> -Dan
>>
>>
>>
>
> I have updated my fix with above suggestions. Thanks for your 
> comments. The webrev is at 
> http://cr.openjdk.java.net/~dxu/7193406/webrev.04/. Thanks!
>
> -Dan

Dan,
I'm Ok with this new webrev, thumb up.

Rémi