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

[Stuart Marks]

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