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

[Dan Xu]

On 09/01/2012 07:11 PM, Ulf wrote:
> Am 01.09.2012 03:15, schrieb Stuart Marks:
>> On 8/31/12 3:19 AM, Ulf Zibis wrote:
>>> Stuart, much thanks for your detailed explanation. The as is 
>>> situation has not
>>> been in question from my side, but anyway it seems, that it had 
>>> catalysed a new
>>> solution, despite that the additional break; could affect JIT 
>>> optimization of
>>> the enclosing loop. So there should be an explaining comment, even 
>>> if Hotspot
>>> (but maybe others) is not affected in current configuration.
>>
>> If we were to add a comment, I'm not sure what it would explain. The 
>> fall-through approach is unusual and is what deserves a comment. 
>> Changing each case to have a break at the end is conventional and 
>> straightforward and doesn't need a comment.
>
> Well, to me it's a kind of taste. The purpose of the given code is to 
> filter all valid delimiters from an invalid tail-match, which are in 
> fact 2 cases, to separate by *one* break.
> So the comment could be:
>          break; // could be dropped to help JIT-optimization, but 
> favours java compiler warning
> or:
>          break; // prevents from java compiler warning, but could risk 
> to miss JIT-optimization threshold
>
> Anyway, the code better should be:
>  527             boolean seencomma = false;
>  528             for (i -= matchlen; i >= 0 && !seencomma; i--) {
>  529                 switch(a[i]) {
>  530                 case ',':
>  531                     seencomma = true;
>  532                     break;
>  533                 case ' ': case '\r': case '\n': case '\f': case 
> '\t':
>  534                     break;
>  535                 default:
>  536                     throw new IllegalArgumentException(
>  537                             "invalid permission: " + actions);
>  538                 }
>  539             }
>  540             // now i points at the location of the comma minus 
> one (or is -1).
>  541         }
>
>
>>>> On 8/30/12 7:14 AM, Ulf Zibis wrote:
>>>>> 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?
>>>
>>> ... You may call me pedantic, but the above question is still open 
>>> to me.
>>
>> I thought I had answered this. But if you think the question is still 
>> open, then perhaps I didn't understand the question.
>
> I wanted to ask, how a switch...case statement, where 
> @SuppressWarnings("fallthrough") could apply, can be enclosed by a 
> declaration, other than a method declaration. Can you give an example?
>

As far as I know, switch statement cannot be embedded inside a simple 
local variable declaration statement. To suppress it, you have to 
annotate at a wider scope, for example the scope of a method or class.

For a complicated local variable declaration statement as the following,

Runnable task = new Runnable() {
     @Override
     public void run() {
         switch statement;
         ...
     }
};

you can annotate the declaration of the variable task. But it is better 
to annotate method run();

>>> Maybe one could optimize javac to recognize such situation and hold 
>>> back the
>>> warning in that case, as the current code looks more intuitive and has
>>> potential advantage for JIT optimization.
>>
>> This seems like a very, very narrow case for the javac compiler to 
>> analyze and to decide whether a warning is truly warranted.
>
> See the following example...
> switch (test) {
>     case "A" : doSomething();
>     case "B" :
>     case "C" : break;
>     default : doOther();
> }
> In pedantic view, case "B" also could be seen as a fallthrough case. 
> So it seems, that javac already suppresses warnings if obviously 
> superfluous.
>
All switch statement can be translated to if-then-else statement.

For example,

char c = ...;
switch(c) {
case 'A': doSomething1(); break;
case 'B':
case 'C': doSomething2(); break;
default: doOther(); break;
}

Translate to:

char c = ...;
if(c == 'A'){
     doSomething1();
} else if(c == 'B' || c == 'C'){
     doSomething2();
} else {
     doOther();
}

In the if-then-else statement, 'A', 'B', and 'C' are not the same level 
conditions. 'B' and 'C' can be regarded as two aspects of one condition. 
But 'A' is a different condition. Thus, it is easy to make the decision 
that missing the break statement in case 'B' is intentional, but missing 
the break statement at the end of case 'A' may be a mistake.

>> It seems unlikely to me that the fall-through approach was a 
>> performance optimization (but if JIT experts want to chime in, please 
>> feel free). It seems likely this was left over from an earlier 
>> version that wanted to share code between the comma and whitespace 
>> cases. When this shared code was removed the fall through was 
>> mistakenly left in place. The files' histories don't shed any light 
>> on this though.
>
> Well, we had the discussion about inserting a local variable 
> declaration to allow a smaller scope for 
> @SuppressWarnings("unchecked"). I believe, that an additional break 
> statement adds additional bytecode too.
As Stuart mentioned earlier, "short methods are the ones most likely to 
be impacted by the addition of a local variable affecting 
compilation/inlining decisions." As this method is sort of long, in my 
understanding, the addition of break statement will not affect the 
compilation result that much. (JIT experts, please correct me if I am 
wrong.)

Thanks,

Dan
>
>> Good point. We had talked about this before (back in December 2011) 
>> but I never filed a bug for it. I've just filed 7195670. (Not sure 
>> when it'll appear on bugs.sun.com though.) Thanks for prompting me to 
>> do this.
>
> Thanks.
>
> -Ulf
>
>