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

[Dan Xu]

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