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

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

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

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

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.

>> This permissions-parsing code occurs in several places around the JDK, .....
>> [...]
>> 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.
>
> Is there already a bug report about that or do you consider to post one?

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.

s'marks