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
