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

[Ulf]

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?

>> 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.

> 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.

> 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