Community
Participate
Working Groups
R3.4. The fall through warning for switch statements is very good and we have it enabled. There is however some good/valid code were this warning is wrong and we would like to have a flag to ignore that. I suggest to add a $FALL-THROUGH$ (like the NON-NLS) tag that tells the compiler that fall through is intended by the programmer. Note that our source has to be 1.4 compliant and hence we cannot use @suppressWarning.
Originally, we had thought of allowing any comment (i.e. signal undocumented fallthrough scenario).
see bug 127730
>Originally, we had thought of allowing any comment (i.e. signal undocumented >fallthrough scenario). You mean if there's a comment you treat it like there would be $FALL-THROUGH$?
Suggest to mark as duplicate of bug 127730 and move bug 127730 back to inbox.
The question is then what comments are counted as "valid fallthrough". In the "undocumented empty block" case, this is easy, but here, we can also have cases where there are other (possibly documented) statements before the fall through. As a heuristic, I would consider a fallthrough case as documented if the last token before the next 'case:' or 'default:' is a comment (line or block comment) and the comment is on its own line. switch(i) { case 0 : //still a fall-through problem System.out.println("very "); //still a fall-through problem case 1 : System.out.println("small "); /* * suppresses the fall-through problem */ case 2 : System.out.println("number"); } See also 67836 comment 2.
Using a heuristic is bad. What do you do in this scenario: switch(i) { case 0 : System.out.println("very "); // need to find out whether adding a break case 1 : fNumber= 1+1; // ^^^ +1 is needed because... case 2 : System.out.println("number"); } ? Just imagine the mess if we would have used heuristic comments to detect non-NLS-ed string.
re: comment 4 I think this one bug as more useful information than bug 127730, so I'd rather keep this one instead. re: specific comment vs. any comment I was more keen on having the notion of documenting in general, like for empty method/class bodies... adding specific tags is more intrusive. The NON-NLS tag is pure legacy, and not a trend. But I do not have a strong opinion at this point.
*** Bug 127730 has been marked as a duplicate of this bug. ***
>I think this one bug as more useful information than bug 127730, so I'd rather >keep this one instead. Agreed. >I was more keen on having the notion of documenting in general, Of course a general story (that does not require 1.5!) would be cool. However we always postponed that solution so far. My goal is to get rid of all problems from the 'Potential Programming Problems' category and so far the only one for which I need a suppress mechanism is this fall-through issue. Hence I'd like to see a specific solution now rather than wait forever for the generic solution ;-)
Created attachment 111838 [details] Patch suggestion Simple solution for testing presence of leading comment (any will do) between last statement and next case. It likely needs to be made more selective in the shape of the comment or its location.
previous patch will consider ANY leading comment as good enough, e.g. e.g. case 3 is no longer blamed public class X { void foo1(int i) { switch(i) { case 0 : case 1 : System.out.println(); // fallthrough doc case 2 : // should not be blamed System.out.println(); // not real fallthrough doc case 3 : // should be blamed System.out.println(); /* fallthrough */ case 4 : // should not be blamed } } }
I am enclined to think it should recognize a specific comment, or at least something in it which is non ambiguous. Anything is just not good enough. Would any comment starting with 'fall-through' do it ?
I would prefer a single comment on its own line as in : case 1 : System.out.println(); // something intelligent case 2 : System.out.println(); break; I don't think we should start forcing the comment to start with a specific phrase unless its like the NLS comment.
> Would any comment starting with 'fall-through' do it ? No. "Comments starting with xyz" is a no-go. Dani's comment 6 makes sense to me, so I would prefer jut $FALL-THROUGH$, as last non-whitespace token in the last comment of a case section. The discussions about the right heuristic for the "any comment" solution show that this will be confusing and difficult to explain, no matter what actual heuristic we chose. And it might also break down when the code formatter or a refactoring decide to add or remove a line break somewhere.
Using a predefined tag also has the advantage that we can easily convert all such instances to something more appropriate later (e.g. some Java annotation based solution).
> (e.g. some Java annotation based solution) Maybe in Java 8, or whenever annotations get supported every statement ;-)
If we want to stick with NON-NLS tag, we would need to detect comment starting strictly with: //$FALL-THROUGH$ At least, it would be the same rule (least surprising). As a future request, we could improve special tag recognition to allow them in non-prefix position, or in other kind of comments (right now only in line comments). Minor: I find $FALL-THROUGH$ to stand out a bit too much in the code actually, what about the following instead ? //$fall-through$
>Minor: I find $FALL-THROUGH$ to stand out a bit too much in the code actually, >what about the following instead ? >//$fall-through$ I suggest to implement it case-insensitive, which allows both patterns.
I would do it the same as for $NON-NLS-1$, i.e. only allow all caps //$FALL-THROUGH$. Even if the compiler allowed lowercase, the quick fix should still generate all caps (for consistency).
Created attachment 111962 [details] Proposed patch Only support capital tag, optionally allow whitespace(s) before the tag.
Proposed patch also tolerates a block comment for the tagging.
> Proposed patch also tolerates a block comment for the tagging. For me, it only tolerates block comments and fails here: switch (1) { case 1: System.out.println(); //$FALL-THROUGH$ case 2: System.out.println(); }
Yes, I realized this. FYI the batch compiler behaves ok for line comment, but some other parser used in IDE reconcile is flushing line comments at this point, leaving the compiler clueless. Will investigate.
Created attachment 112048 [details] Better patch Issue came from negated commentStart in downstream parser implementations (see bug 246682)
Created attachment 112049 [details] Better patch Better patch for real
Created attachment 112057 [details] Better patch also tweaked some regression tests after change in problem message
Released for 3.5M2 Fixed
Improved compiler problem message again: Switch case may be entered by falling through previous case. If intended, insert a //$FALL-THROUGH$ comment just before
And again: Switch case may be entered by falling through previous case. If intended, add a new comment //$FALL-THROUGH$ on the above line
And again: Switch case may be entered by falling through previous case. If intended, add a new comment //$FALL-THROUGH$ on the line above
Verified for 3.5M2 using I20080915-1800
