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

[David Holmes]

On 1/09/2012 5:42 AM, Mike Duigou wrote:
> The changes look good to me.
>
> I am starting to come into agreement with Remi though that unless a type specific array can be created for situations like the E[] elements array in ArrayDeque then it should be declared as Object[] array since that's what is actually created.

The fact we can not create an E[] is a limitation of generics. The 
elements should be an E[] though. Better one cast on array creation than 
a cast on every element access.

David

> Mike
>
> On Aug 31 2012, at 12:06 , Dan Xu wrote:
>
>> On 08/30/2012 06:45 PM, Stuart Marks wrote:
>>> On 8/30/12 7:14 AM, Ulf Zibis wrote:
>>>> Am 30.08.2012 01:20, schrieb Stuart Marks:
>>>>> On 8/29/12 4:36 AM, Ulf Zibis wrote:
>>>>>>> @SuppressWarnings("fallthrough") is put to suppress warnings generated by
>>>>>>> another switch/case statements
>>>>>> Can't you move it from method scope to there?
>>>>>>
>>>>>>>             while (i>= matchlen&&  !seencomma) {
>>>>>>>                 switch(a[i-matchlen]) {
>>>>>>>                 case ',':
>>>>>>>                     seencomma = true;
>>>>>>>                     /*FALLTHROUGH*/
>>>>>>>                 case ' ': case '\r': case '\n':
>>>>>>>                 case '\f': case '\t':
>>>>>>>                     break;
>>>>>>>                 default:
>>>>>>>                     throw new IllegalArgumentException(
>>>>>>>                             "invalid permission: " + actions);
>>>>>>>                 }
>>>>>>>                 i--;
>>>>>>>             }
>>>>>
>>>>> Unfortunately there is no suitable place to put the annotation. Annotations
>>>>> can only be placed on declarations, and the narrowest enclosing declaration
>>>>> around this switch statement is, unfortunately, the entire method. One might
>>>>> consider refactoring the switch statement into its own method, but that kind
>>>>> of refactoring is too large a change for warnings cleanup.
>>>>
>>>> I can see that it is reasonable/possible to bind a
>>>> @SuppressWarnings("unchecked") to a declaration which contains the unchecked
>>>> assignment, but which declaration should contain a fallthrough case?
>>>>
>>>> So shouldn't  @SuppressWarnings("fallthrough") better be binded to a switch or
>>>> case statement?
>>>
>>> In principle it would be nice to place the @SuppressWarnings right next to the switch statement for which the warnings are suppressed. However, it is a syntactic restriction of the Java programming language that annotations can only be used as modifiers on declarations, specifically package, class, method, method parameter, constructor, and local variable declarations. See JLS 9.7 [1]. It is not legal to place annotations on statements. So, the SuppressWarnings annotation has to be placed on a declaration that encloses the switch statement in question. In this case that's the declaration of the entire method.
>>>
>>> [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.7
>>>
>>> It turns out that this discussion is moot, as the switch fall-through is actually unnecessary! The comma case falls through to the next case, but all that does is break. The warning can be eliminated simply by replacing /*FALLTHROUGH*/ with a break statement. This removes the need for the SuppressWarnings annotation.
>>>
>>> This permissions-parsing code occurs in several places around the JDK, copied and slightly modified. Some of them use the string identity comparison optimization, and some have this switch fall through. (We've discussed this previously. See [2].) Accordingly, I've asked Dan to adjust a couple of these other cases to have the same comment about string identity comparisons, and to change the switch statement to avoid the fall-through entirely.
>>>
>>> [2] http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000415.html
>>>
>>> While I'm reluctant to expand the scope of this changeset, I think it's important to keep these five occurrences consistent. Obviously it would be nicer to unify the five copies into one, but this is a much bigger change that I think is clearly out of scope.
>>>
>>> Dan will issue another webrev with these changes shortly.
>>>
>>> s'marks
>>>
>>
>> I have adjusted my fix with the above suggestions.
>>
>> The latest webrev is posted at, http://cr.openjdk.java.net/~dxu/7193406/webrev.05/. Thanks for your comments!
>>
>> -Dan
>