Community
Participate
Working Groups
With 3.8M6 the following code produces the warning "The switch on the enum type Test.TestEnum should have a default case". Why is default case necessary if all enum values are covered by the switch? public class Test { private static enum TestEnum { ONE, TWO } public void test(TestEnum p) { switch (p) { case ONE: break; case TWO: break; } } } The warning is new in M6 and was not present in M5.
This warning is by design. In fact the JLS recommends that compilers should report such. For references please see - bug 265744 - JLS 14.11, which says "A Java compiler is encouraged (but not required) to provide a warning if a switch on an enum-valued expression lacks a default label ..." - Eclipse 3.8 and 4.2 M6 - New and Noteworthy http://download.eclipse.org/eclipse/downloads/drops4/S-4.2M6-201203151300/eclipse-news-M6.html#incomplete-switch-over-enum To answer your question: (In reply to comment #0) > Why is default case necessary if all enum values are covered by the switch? The default is not strictly necessary (hence only a configurable warning) but specifying a default case is good practice to improve binary compatibility. Additionally, the New&Noteworthy mentions a related situation where a default case is strictly required by the JLS.
One more reference with some nice discussion in this field: http://stackoverflow.com/questions/5013194/why-is-default-required-for-a-switch-on-an-enum-in-this-code
Unfortunately, this change sacrifices compile-time check that all enum values are covered by the switch for a formal, but not necessarily semantically valid, binary compatibility. If the code is changed to have a default case, omitting one of enum-value cases does not produce a warning: public void test(TestEnum p) { switch (p) { case ONE: break; default: break; } } We've lost an important mechanism forcing programmer to update switch statements that depend on the values of an enum. Please make the new behavior configurable.
(In reply to comment #3) > Unfortunately, this change sacrifices compile-time check that all enum values > are covered by the switch for a formal, but not necessarily semantically valid, > binary compatibility. If the code is changed to have a default case, omitting > one of enum-value cases does not produce a warning: > > public void test(TestEnum p) { > switch (p) { > case ONE: > break; > default: > break; > } > } > > We've lost an important mechanism forcing programmer to update switch > statements that depend on the values of an enum. > > Please make the new behavior configurable. Right, we should fix bug 132917.
(In reply to comment #3) > Unfortunately, this change sacrifices compile-time check that all enum values > are covered by the switch for a formal, but not necessarily semantically valid, > binary compatibility. So, to clarify: only after changing the code to fix the new warning, one has lost the functionality to get warned about missing case labels.
Good points. I think we should split the option in two: a) "Enum type constant not covered on 'switch'": Warns when a switch statement doesn't contain explicit cases for all constants, regardless of a default case. => This is the old compiler.problem.incompleteEnumSwitch. b) "Missing 'default' case on switch": Should work on any kind of switches, not only on enums. The argument about compatibility with new code that has more possible cases also holds for integer-based switches.
(In reply to comment #5) > So, to clarify: only after changing the code to fix the new warning, one has > lost the functionality to get warned about missing case labels. Yes.
(In reply to comment #6) > Good points. I think we should split the option in two: > > a) "Enum type constant not covered on 'switch'": Warns when a switch statement > doesn't contain explicit cases for all constants, regardless of a default case. > => This is the old compiler.problem.incompleteEnumSwitch. Agree with this part. > b) "Missing 'default' case on switch": Should work on any kind of switches, not > only on enums. The argument about compatibility with new code that has more > possible cases also holds for integer-based switches. I am not sure about this part: While verifying the fix for bug 265744, I also misread the JLS wording quoted in bug 265744 comment#0, reproduced below: "Compilers are encouraged (but not required) to provide a warning if a switch on an enum-valued expression lacks a default case and lacks cases for one or more of the enum type's constants..." In reality, NO warning is suggested for MERE absence of a default case. Only when it is ALSO the case (:)) that one or more of the enum type constants do(es) not feature in the case statements is a warning suggested. Moreover, it is not clear to me at all that the claim made in bug 265744 comment#0's regarding "JLS 16.2.9 requiring a default label no matter what is the switch type (int or enum)" is quite correct. This section merely states the rules of definite assignment as it pertains to a switch statement and what the absence of a default means there. If something is not definitely assigned after a switch then that is just so. In the following, would we argue that there should be a warning "missing else" clause ? //----------------------------- public class X { public void test(int x) { int p; if (x == 10) { p = x; } else if (x == 20) { p = x; } System.out.println(p); } } These questions should have been asked earlier - Sorry,l. my fault, I took the JLS encouragement part at face value and my misreading added to the mix. One possible course of action: - Leave the UI label as is. The changed label is still good. - Leave the flipped defaults as is - it is still a good thing. - Pull out the warning for missing default. - Fix the documentation.
(In reply to comment #8) > - Pull out the warning for missing default. i.e Leave the API problem constant IProblem.MissingEnumDefaultCase as is. Just don't issue the missing default warning at all and fix the documentation where required.
(In reply to comment #6) > b) "Missing 'default' case on switch": Should work on any kind of switches, not > only on enums. The argument about compatibility with new code that has more > possible cases also holds for integer-based switches. If we do split it this way, the warning for missing default should be off by default. Otherwise, the perceived nuisance value will be high I fear. Outside of the definite assignment related cases and certain control flow related cases exemplified by https://bugs.eclipse.org/bugs/show_bug.cgi?id=149562 https://bugs.eclipse.org/bugs/show_bug.cgi?id=207915 https://bugs.eclipse.org/bugs/show_bug.cgi?id=97790 the new warning will simply be seen as a nuisance as exemplified by the code from comment#0 which does not involve any definite assignment related issued for example.
The problem seems to be more involved. Let's look at the scenarios at hand: a) Warn on switches that currently don't handle all enum values. => That's JLS 14.11 and our old implementation. - Warning condition: "missing default case" && "at least one missing enum case" - Emitted warnings: one warning per missing enum case b) Warn on switches that don't have explicit case statements for all current enum values. => That's the request in bug 132917 and this would avoid the loss in comment 3. - Warning condition: "at least one missing enum case" - Emitted warnings: one warning per missing enum case c) Warn on switches that miss a default case => Intention behind bug 265744 for binary compatibility reasons and to align with definite assignment rules. - Warning condition: "missing default case" - Emitted warnings: one warning for missing default Bug 265744 turned the compiler option into an irregular combination of (b) and (c), i.e.: - Warning condition: "missing default case" [from (c)] - Emitted warnings: one warning for missing default [from (c)] + one warning per missing enum case [from (b)] What's the value of the scenarios? ---------------------------------- (a) is a valuable diagnostic if you care about complete coverage of enum constants in the current code but don't care about evolution. Both (b) and (c) are valuable diagnostics if you care about evolution of the code: - (b) ensures that all enum constants (old and new) are handled explicitly => helps to detect known evolution of enum and missing cases. Pretends that there's no 'default:' at all. - (c) ensures that new enum constants are handled gracefully (not skipped) => helps for binary compatibility if enum evolves independently of client code Recommendation: --------------- Revert the old option to the old implementation but set it to warning by default and keep the new name. Add two new options like this: Default: "Incomplete 'switch' cases on enum" [E/W/I] -- warning [old] [ ] "Report even if 'default:' is present" -- off [new for (b)] "Missing 'default:' on 'switch'" [E/W/I] -- ignore [new for (c)] => That way, we behave as recommended in JLS 14.11 by default, but we also support the evolution scenarios. Confession: The checkbox for (b) is not a masterpiece, since it requires redundant case labels in the code. But that's the only way to detect switch statements that had been written before new enum constants were added.
(In reply to comment #11) > The problem seems to be more involved. Yes, Let us make haste slowly. > b) Warn on switches that don't have explicit case statements for all current > enum values. > => That's the request in bug 132917 and this would avoid the loss in comment I can see the value in this warning. bug 132917 should not have been closed as WONTFIX - In fact, it was not closed consciously as such, but was closed in the administrative blitzkrieg that changed LATER and REMIND resolution kinds - hence I support reopening it and addressing it. > c) Warn on switches that miss a default case > => Intention behind bug 265744 for binary compatibility reasons and to align > with definite assignment rules. I would like to understand where and how binary compatibility entered the discourse - is this from bug 207915 comment# 1 ? That is a valid comment of course, but is a rationalization for some *other* diagnostic. OIOW, can we construct a test case where we would issue the new missing default warning, which test case **would not also** elicit some other diagnostic triggered by the rules governing definite assignment and/or reachability ? That would be the litmus test for the new missing default warning's usefulness. If such a test case doesn't exist, the new warning is redundant at best and will be perceived to be a nuisance at worst. > Both (b) and (c) are valuable diagnostics if you care about evolution of the > code: [...] > - (c) ensures that new enum constants are handled gracefully (not skipped) > => helps for binary compatibility if enum evolves independently of client > code Per above, the point about (c) holds only if, this warning in and off itself serves this purpose. If this can be ascertained to be the case, I am fine the recommendation Markus has put together. Even otherwise the recommendation is fine - only I would like us to deliberately and consciously arrive at that decision. Sorry for the misunderstanding - I hadn't realized that M5 implementation is THE implementation of JLS 14.11 and assumed the fix for bug 265744 is filling the (what now proves to be the non-existent) gap.
(In reply to comment #12) > OIOW, can we construct a test case where we would issue the new > missing default warning, which test case **would not also** elicit > some other diagnostic triggered by the rules governing definite > assignment and/or reachability ? To spell out a little bit more explicitly (to forestall confusion): Can we construct a test case where we would issue the new missing default warning as a means to call out concerns around binary compatibility, which test case **would not also** elicit some other diagnostic triggered by the rules governing definite assignment and/or reachability ? That would be the litmus test for the new missing default warning's usefulness.
(In reply to comment #12) > OIOW, can we construct a test case where we would issue the new > missing default warning, which test case **would not also** elicit > some other diagnostic triggered by the rules governing definite > assignment and/or reachability ? Yes, see comment 0. Definite assignment and reachability only take part in the game if you deal with local variables, or if each case ends with a "return x;" statement and the switch statement is the last statement of the method. If the case statements use other means to do their job (e.g. assign to a field or call different methods), then the absent 'default:' is not detected.
(In reply to comment #14) > (In reply to comment #12) > > OIOW, can we construct a test case where we would issue the new > > missing default warning, which test case **would not also** elicit > > some other diagnostic triggered by the rules governing definite > > assignment and/or reachability ? > Yes, see comment 0. OK, I think orderly evolution and binary compatibility are intertwined yet separate ideas and we are drawing lines at different points. Assume that TestEnum from comment#0 was declared in a separate file. Adding another case to the enum does not break the class Test from a binary compatibility stand point per se - JLS 13.4.26 explicitly states that "Adding or reordering constants from an enum type will not break compatibility with pre-existing binaries". I agree it can still cause some surprises due to independent evolution and partial rebuild and this scenario is worth supporting with a warning. > Definite assignment and reachability only take part in the game if you deal > with local variables, or if each case ends with a "return x;" statement and the > switch statement is the last statement of the method. These are the cases where I see binary compatibility issues come in from a language stand point to be able to guarantee that the program transitions through well defined set of states and to forbid scenarios where such guarantee would be violated due to independent evolution and partial rebuild. For example: // --- X.java: public class X { public static void test(TestEnum p) { int ordinal; switch (p) { case ONE: ordinal = p.ordinal(); break; case TWO: ordinal = p.ordinal(); break; default: ordinal = -1; break; } System.out.println(ordinal); } public static void main(String[] args) { TestEnum t = TestEnum.ONE; test(t); } } // -------- TestEnum.java public enum TestEnum { ONE, TWO }; If the default clause was not there AND if the language allowed X to compile without the default clause alright, then we will surely have a problem with X breaking if and when a new enumerator is added. (See that X gets loaded and verified ahead of TestEnum) After fixing this difference in my mind and looking at the both the scenarios, I am fine with supporting both and so the recommendation from comment#11 looks good to me. This requires an API change though - Dani, what are your thoughts on that ?
> This requires an API change though - Dani, what are your thoughts > on that ? Once all parties agree on the solution you can post an API change request to the PMC mailing list. If we only add new APIs or change something we added during 3.8 then I see no problem approving it.
Yes, in this bug, the term "binary compatibility" is not strictly used in the JLS sense of the term. It's more about "correctness of a switch statement when the enum evolved independently and the switch can have a good default implementation that likely makes sense for all new enum constants".
(In reply to comment #17) Since "good default implementation that likely makes sense for all new enum constants" does not apply to all switch statements, the corresponding warning is never appropriate. Oonly a human can judge if the "good default implementation" exists or not.
(In reply to comment #18) So your vote is to omit the "Missing 'default:' on 'switch'" [E/W/I] for (c)? I disagree with that. For some enums, a human can judge that there's likely a good action for new enum constants. E.g. if java.lang.annotation.ElementType.TYPE_PARAMETER gets added, then a processor may choose to clean up and then do nothing, or it may choose to throw an exception. The warning is there to tell the human that there is a choice to make.
(In reply to comment #19) > (In reply to comment #18) > > So your vote is to omit the "Missing 'default:' on 'switch'" [E/W/I] for (c)? I abstain. I don't see how this warning can be implemented to be helpful, but I don't want to force my opinion on other people. > I disagree with that. For some enums, a human can judge that there's likely a > good action for new enum constants. E.g. if > java.lang.annotation.ElementType.TYPE_PARAMETER gets added, then a processor > may choose to clean up and then do nothing, or it may choose to throw an > exception. The warning is there to tell the human that there is a choice to > make. Consider the following scenario: 1. Eclipse tells me that there is a choice to make. 2. I look at the code and choose not to add the default case because "good default implementation that likely makes sense for all new enum constants" doesn't exist. 3. Now, how do I tell Eclipse to not bother me again with the warning? I also don't want other people to be bothered with the warning because I already spent time thinking about the switch and don't want other developers to waste time on it.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > > So your vote is to omit the "Missing 'default:' on 'switch'" [E/W/I] for (c)? > > I abstain. I don't see how this warning can be implemented to be helpful, but I > don't want to force my opinion on other people. Curiosity question: Is your position the same if the missing default warning is limited only to enum switches ? (comment# 11 recommendation is for all switches I believe.) I like the pattern of using defaults to throw exceptions outlined in https://bugs.eclipse.org/bugs/show_bug.cgi?id=132917 but personally I am not convinced of the value of missing default switch warnings in the non-enum scenarios, but can live with that since it is proposed to be off by default. May be we should gather data on eclipse code base itself to see how many warnings would be produced and how we would "fix" them to gauge the "nuisance" factor ? Stephan, could you please look into this ? Thanks. Also once there is convergence, please post the API changes required included javadoc etc, so it can be discussed at the pmc list - TIA. > > > I disagree with that. For some enums, a human can judge that there's likely a > > good action for new enum constants. E.g. if > > java.lang.annotation.ElementType.TYPE_PARAMETER gets added, then a processor > > may choose to clean up and then do nothing, or it may choose to throw an > > exception. The warning is there to tell the human that there is a choice to > > make. > > Consider the following scenario: > 1. Eclipse tells me that there is a choice to make. > 2. I look at the code and choose not to add the default case because "good > default implementation that likely makes sense for all new enum > constants" doesn't exist. > 3. Now, how do I tell Eclipse to not bother me again with the warning? I also > don't want other people to be bothered with the warning because I already spent > time thinking about the switch and don't want other developers to waste time on > it.
(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #19) > > > (In reply to comment #18) > > > > > > So your vote is to omit the "Missing 'default:' on 'switch'" [E/W/I] for (c)? > > > > I abstain. I don't see how this warning can be implemented to be helpful, but I > > don't want to force my opinion on other people. > > Curiosity question: Is your position the same if the missing default > warning is limited only to enum switches ? (comment# 11 recommendation > is for all switches I believe.) Everything I've said applies to enum switches only. This is where the new warning hurts me. > I like the pattern of using defaults to throw exceptions outlined in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=132917 but personally > I am not convinced of the value of missing default switch warnings in > the non-enum scenarios, but can live with that since it is proposed to > be off by default. Adding default case to the example in comment #0 eliminates the error or warning that would be produced if an additional value were added to the enum. I prefer not to add the default case because I cherish the enum coverage warning/error. One possible way to have the coverage warning and the default case, both at the same time, is to introduce a special comment: switch (p) { case ONE: break; case TWO: break; /* DOES_NOT_COMPENSATE_FOR_MISSING_ENUM_VALUES */ default: throw ... } The quick fix for adding default case could include the special comment, so nobody would have to type it by hand. I'm not a big fan of this solution though since the code looks pretty ugly. > May be we should gather data on eclipse code base itself to see how > many warnings would be produced and how we would "fix" them to gauge the > "nuisance" factor ? I filed this bug because the "nuisance" factor on CDT code base is pretty high.
(In reply to comment #22) > Adding default case to the example in comment #0 eliminates the error or > warning that would be produced if an additional value were added to the enum. I > prefer not to add the default case because I cherish the enum coverage > warning/error. But this is already addressed in the recommendation made in comment#11 i.e There will be a way to get the compiler to warn of missing enumerator cases even when the default is present.
(In reply to comment #23) > But this is already addressed in the recommendation made in comment#11 > i.e There will be a way to get the compiler to warn of missing enumerator > cases even when the default is present. Unconditional warning on missing enum values in the presence of default case is wrong. Consider a scenario when the enum contains 100 values, but only two of them require special handling. A warning like that can be useful only if it can be enabled/disabled on per-switch basis.
(In reply to comment #21) > May be we should gather data on eclipse code base itself to see how > many warnings would be produced and how we would "fix" them to gauge the > "nuisance" factor ? > > Stephan, could you please look into this ? Thanks. Also once there is > convergence, please post the API changes required included javadoc etc, > so it can be discussed at the pmc list - TIA. Stephan, this item is waiting to be weighed in by you. TIA.
Since the new behavior is a regression, the commit that introduced it should be reverted until a correct solution is ready.
(In reply to comment #26) > Since the new behavior is a regression, the commit that introduced it should be > reverted until a correct solution is ready. I would agree if this were to be a build breakage, bad code generation or data corruption kind of problem or if the fix is going to be delayed across milestones. The current issue at hand concerns optional behavior that can be justifiably seen to be nuisance per viewpoint, but something that can be ignored until a fix is made. It is anyways targeted for M7 and we will try to fix it as soon as possible - the "delay" is only because of eclipsecon coming in between and some cycles needing to be devoted to that. FWIW, there have also not been successful I builds over the last couple of weeks due to the build set up transmigration. Thanks for your patience.
(In reply to comment #27) > The current issue at hand concerns optional behavior that can be justifiably > seen to be nuisance per viewpoint, but something that can be ignored > until a fix is made. I strongly disagree. Regression is always bad, no matter the scale of damage it creates.
We will fix this for M7 and not back out any code.
(In reply to comment #29) > We will fix this for M7 and not back out any code. What worries me is that there is still no workable solution proposed. The one in comment #11 is flawed for reasons explained in comment #20 and comment #22.
I mostly agree with comment 11, so let me discuss a few issues relative to that proposal. (In reply to comment #19) > ... For some enums, a human can judge that there's likely a > good action for new enum constants. E.g. if > java.lang.annotation.ElementType.TYPE_PARAMETER gets added, then a processor > may choose to clean up and then do nothing, or it may choose to throw an > exception. The warning is there to tell the human that there is a choice to > make. I agree. I understand Sergey's rebuttal as asking for a way how to persist such human choice. In reaction to a "missing default" warning I see three good options: - add a default to give a meaningful implementation for future enum values - add a default to abort when encountering an unexpected value - add an empty default to document that no useful default action can be implemented With the option to report missing cases despite a default label I think this warning is perfectly actionable. How about "missing case" warnings? (In reply to comment #24) > Unconditional warning on missing enum values in the presence of default case is > wrong. Consider a scenario when the enum contains 100 values, but only two of > them require special handling. A warning like that can be useful only if it can > be enabled/disabled on per-switch basis. Yes, unconditional warning may be a bit harsh. Maybe we need a way to tell the compiler when an enum switch is intentionally incomplete, like: switch (dayOfTheWeek) { case SATURDAY: cleanHouse(); break; case SUNDAY: celebrate(); break; //$CASES-OMITTED$ default: work(); break; } This comment would suppress any warnings about missing cases (if that is enabled). Let's look at it this way: Java has no distinction between a default containing useful implementation for all omitted cases and a default as an emergency handler. Using a $CASES-OMITTED$ marker, we could signal the first kind of default, otherwise we assume that default: is just an emergency handler (i.e., all enum constants should be explicitly covered). From all I can see this proposal remedies all remaining concerns raised. (In reply to comment #11) > [ ] "Report even if 'default:' is present" -- off [new for (b)] I agree that we are over-eager currently. Unfortunately, off-by-default will not help those who are puzzled about a flow-related error caused by a missing default case. Here's a somewhat unusual proposal: * If this situation occurs in a method that also has one of the flow related errors (uninitialized variable, missing return), warn about missing default *even* if this option is off (as a possible explanation for the error. The warning will disappear, once the error has been fixed). (In reply to comment #21) > Stephan, could you please look into this ? Thanks. Also once there is > convergence, please post the API changes required included javadoc etc, > so it can be discussed at the pmc list - TIA. Sure. Just let me know what you think of my two amendments to Markus' proposal from comment 11: - support a $CASES-OMITTED$ comment tag to suppress warnings against missing cases - conditionally enable "missing cases" warning in the presence of related flow errors.
(In reply to comment #31) I like the Stephan's proposal. > * If this situation occurs in a method that also has one of the flow related > errors (uninitialized variable, missing return), warn about missing default > *even* if this option is off (as a possible explanation for the error. The > warning will disappear, once the error has been fixed). I wonder it it's feasible to include a note about the missing default case into the error message for the flow-related error as opposed to having it separate. In any case, if combining the messages is too hard, keeping them separate is good enough.
(In reply to comment #31) > Yes, unconditional warning may be a bit harsh. Maybe we need a way to tell the > compiler when an enum switch is intentionally incomplete, like: > > switch (dayOfTheWeek) { > case SATURDAY: cleanHouse(); break; > case SUNDAY: celebrate(); break; > //$CASES-OMITTED$ > default: work(); break; > } > > This comment would suppress any warnings about missing cases (if that is > enabled). > > Let's look at it this way: Java has no distinction between a default containing > useful implementation for all omitted cases and a default as an emergency > handler. Using a $CASES-OMITTED$ marker, we could signal the first kind of > default, otherwise we assume that default: is just an emergency handler (i.e., > all enum constants should be explicitly covered). I like this proposal. There is prior art in the form of $FALL-THROUGH$" tags in the very same context. > (In reply to comment #11) > > [ ] "Report even if 'default:' is present" -- off [new for (b)] > > I agree that we are over-eager currently. Unfortunately, off-by-default will > not help those who are puzzled about a flow-related error caused by a missing > default case. Here's a somewhat unusual proposal: > * If this situation occurs in a method that also has one of the flow related > errors (uninitialized variable, missing return), warn about missing default > *even* if this option is off (as a possible explanation for the error. The > warning will disappear, once the error has been fixed). I don't like this part. (a) This is "irregular" design IMHO. This is an unwarranted instance of the compiler trying to be smarter than the programmer by trying its hand at divination (b) The absence of default in some switch in the method may have nothing to at all with the flow related error in the method. (b) Equivalently, a switch may have a default and still result in a flow error because the default clause is not doing "its part" to ensure that control flow through that path satisfies some condition (e.g initializes some variables ...)
(In reply to comment #31) Please consider couple refinements to Stephan's proposal: 1. Don't warn about missing default case if all cases are covered and the enum is defined in the same source file as the switch statement. 2. Formulate the message for the missing default case warning differently depending on whether all enum values are covered or not.
(In reply to comment #33) > I don't like this part. (a) This is "irregular" design IMHO. This is > an unwarranted instance of the compiler trying to be smarter than the > programmer by trying its hand at divination (b) The absence of default > in some switch in the method may have nothing to at all with the flow > related error in the method. (b) Equivalently, a switch may have a > default and still result in a flow error because the default clause is not > doing "its part" to ensure that control flow through that path satisfies > some condition (e.g initializes some variables ...) Srikanth, what do you think of including a note about the missing default case into the error message for the flow-related error as opposed to having it separate? This should happen only if the flow-related error could be fixed by adding a default case with some code.
(In reply to comment #34) > (In reply to comment #31) > Please consider couple refinements to Stephan's proposal: > 1. Don't warn about missing default case if all cases are covered and the enum > is defined in the same source file as the switch statement. This looks reasonable to me. Shouldn't also be difficult. > 2. Formulate the message for the missing default case warning differently > depending on whether all enum values are covered or not. This should be doable without too much effort - would like to see some articulation on why this would be useful/valuable though. (for the record) (In reply to comment #35) > (In reply to comment #33) > Srikanth, what do you think of including a note about the missing default case > into the error message for the flow-related error as opposed to having it > separate? This should happen only if the flow-related error could be fixed by > adding a default case with some code. AFAICT, we don't have the infrastructure/machinery to form such inferences in a clear cut and categorical fashion and it is not worth building such machinery for the current issue. The value of the warning on missing defaults to explain some otherwise not immediately obvious flow related errors and be documented well thereby encouraging users to turn on this warning - but if this warning is off, we should not emit this warning.
(In reply to comment #36) > (In reply to comment #34) > > (In reply to comment #31) > > Please consider couple refinements to Stephan's proposal: > > 1. Don't warn about missing default case if all cases are covered and the enum > > is defined in the same source file as the switch statement. > > This looks reasonable to me. Shouldn't also be difficult. Agreed. > > 2. Formulate the message for the missing default case warning differently > > depending on whether all enum values are covered or not. > > This should be doable without too much effort - would like to see some > articulation on why this would be useful/valuable though. (for the record) I don't think this differentiation is very important, but I'm open to proposals how to express this difference. > (In reply to comment #35) > > (In reply to comment #33) > > > Srikanth, what do you think of including a note about the missing default case > > into the error message for the flow-related error as opposed to having it > > separate? This should happen only if the flow-related error could be fixed by > > adding a default case with some code. > > AFAICT, we don't have the infrastructure/machinery to form such inferences > in a clear cut and categorical fashion and it is not worth building such > machinery for the current issue. Right, we don't have the machinery to definitely say: "this flow error is caused by the lack of that default case", but we should give a hint about a possible correlation, like "... this could be caused by the lack ..." or "... for further hints please consider enabling all warnings regarding switch statements lacking a default case...", or "... please note that this method also has an incomplete enum-switch which might explain this error." It's about giving a hint to a helpless user. If we see both problems (flow and missing default) in the same method, we have information that is valuable because it potentially connects two bits that the typical user sees as unrelated. Dropping this information because the user is not aware of the relevance of one warning would be a great pity, IMO. If such correlation should not be used for reporting I request to leave the warning for missing default case at "warning-by-default" to preserve the intention behind the fix for bug 265744.
(In reply to comment #36) > > 2. Formulate the message for the missing default case warning differently > > depending on whether all enum values are covered or not. > > This should be doable without too much effort - would like to see some > articulation on why this would be useful/valuable though. (for the record) The main reason for the differentiation is to help user understand the warning. Currently the warning says: "The switch on the enum type MyEnum should have a default case". We can keep this wording for the case when there are missing enum cases. When all enum cases are already covered, we can say "The switch on the enum type MyEnum should have a default case to accommodate new values that may be added to MyEnum in future". Alternative wording suggestions are welcome.
(In reply to comment #37) > Right, we don't have the machinery to definitely say: "this flow error is > caused by the lack of that default case", but we should give a hint about a > possible correlation, like "... this could be caused by the lack ..." or "... > for further hints please consider enabling all warnings regarding switch > statements lacking a default case...", or "... please note that this method > also has an incomplete enum-switch which might explain this error." It's about > giving a hint to a helpless user. I just want to warn that it is likely such messages are useful in some situations and are likely unhelpful in others and send the user in wrong directions. As comment#8 points out, this same situation can very well happen in an if-else-if ladder. > If such correlation should not be used for reporting I request to leave the > warning for missing default case at "warning-by-default" to preserve the > intention behind the fix for bug 265744. I think this is better opted in for due to the perceived noise value: see comment# 22.
(In reply to comment #39) > I just want to warn that it is likely such messages are useful in some > situations and are likely unhelpful in others and send the user in wrong > directions. > > As comment#8 points out, this same situation can very well happen in > an if-else-if ladder. I am away for the next 10 days, so you can take a call on this after hearing opinions from other stakeholders. I am not dead against what you propose, so if you hear supporting views, please go ahead without waiting for me to get back and comment.
(In reply to comment #39) > (In reply to comment #37) > > > Right, we don't have the machinery to definitely say: "this flow error is > > caused by the lack of that default case", but we should give a hint about a > > possible correlation, like "... this could be caused by the lack ..." or "... > > for further hints please consider enabling all warnings regarding switch > > statements lacking a default case...", or "... please note that this method > > also has an incomplete enum-switch which might explain this error." It's about > > giving a hint to a helpless user. > > I just want to warn that it is likely such messages are useful in some > situations and are likely unhelpful in others and send the user in wrong > directions. > > As comment#8 points out, this same situation can very well happen in > an if-else-if ladder. The difference being: no-one should expect that the compiler analyses completeness of an if-else-if ladder without final else. By contrast, we give warnings about missing cases in an enum-switch until all constants are covered - thus signalling: the switch is now complete; regarding flow analysis this signal is wrong. That's the kind of (JLS-incurred) confusion I want to attenuate with the warning.
I just discovered this issue here after downloading latest juno build and found this huge thread about something that working perfectly fine before. From an eclipse user's perspective, this what I'd like to see eclipse do: 1. have an option to warn if a switch for an enum doesn't cover all cases. 2. have an option to warn if other switch statements do not have a default case The rationale is simple: I want to know if today I'm not covering all possible values in the switch statement. I don't care if in the future I may not cover all cases. For non-enums, it's generally impossible for the compiler to tell if one is handling all values, and so a default should be added, if only to throw an AssertionError or IllegalArgumentException. I'm actually setting the option with ERROR, because I want my code to break if I add a new enum value. When I add a new constant, all the code breaks and I quickly fix it up. If I had a default statement, then I may forget to fix up a switch statement (I'd have to find all usages of the enum to verify that I'm not missing anything). I guess you could consider it a workaround for missing refactor feature.
Created attachment 213871 [details] draft patch v0.1 Draft of what's cooking.
Created attachment 213872 [details] mylyn/context/zip
Here's a summary of the approach taken in the patch (comment 43): API in JavaCore: COMPILER_PB_INCOMPLETE_ENUM_SWITCH [E/W/I]: This is basically the old warning pre M6, but default is now WARNING COMPILER_PB_MISSING_ENUM_CASE_DESPITE_DEFAULT [ENABLED/DISABLED] Boolean switch to also warn about missing enum constants despite an existing default case Modifies the scope of the above option. Default: DISABLED. COMPILER_PB_MISSING_DEFAULT_CASE [E/W/I]: For all kinds of switch report missing default case. Default: IGNORE Up-to this point this should match/support the proposal in comment 11. Internally IProblems and problem messages are a bit more fine grained: Missing enum constant, depending on presence of a default case (761=no default, 768=with default): 761 = The enum constant {1} needs a corresponding case label in this enum switch on {0} 768 = The enum constant {1} should have a corresponding case label in this enum switch on {0} Missing default, depending on the type: 766 = This enum switch should have a default case to account for future additions to the enum type {0} 767 = The switch over the type {0} should have a default case All are suppressable using token "incomplete-switch". Next I added the option to silence warnings by writing //$CASED-OMITTED$ before the default case => no warnings re missing enum constant cases in this switch. One detail is slightly unclear to me: what if this tag meets a //$FALL-THROUGH$. I think a sequence of both should be supported, however, my first go at making this work broke two tests in SwitchTest (test014, test015), but that's a minor issue. Finally, I internally record this situation: - all enum constants are covered AND - missing default case is detected AND - this warning is set to "ignore". IF in this situation a flow-related error is reported for which the ignored warning could be a valuable hint, THEN the flow problem is modified like so: 769 = The local variable {0} may not have been initialized. Note that a problem regarding missing 'default:' on 'switch' has been suppressed, which is perhaps related to this problem 770 = The blank final field {0} may not have been initialized. Note that a problem regarding missing 'default:' on 'switch' has been suppressed, which is perhaps related to this problem 771 = This method must return a result of type {0}. Note that a problem regarding missing 'default:' on 'switch' has been suppressed, which is perhaps related to this problem From all I can see this is the most flexible approach serving a wide range of requirements and use cases. I'd like to ask permission for these API changes: JavaCore: - modify (partly revert actually) semantics of COMPILER_PB_INCOMPLETE_ENUM_SWITCH - add new constants COMPILER_PB_MISSING_ENUM_CASE_DESPITE_DEFAULT and COMPILER_PB_MISSING_DEFAULT_CASE IProblem: - add new constants MissingEnumDefaultCase, MissingDefaultCase, MissingEnumConstantCaseDespiteDefault, UninitializedLocalVariableHintMissingDefault, UninitializedBlankFinalFieldHintMissingDefault, ShouldReturnValueHintMissingDefault Javadoc has been updated in the patch accordingly. Regardless of the above defaults I would personally encourage users to enable all warnings and use $CASES-OMITTED$ to document intentional omissions. With this strategy the compiler will give optimal support for writing code that's safe now and for evolution.
(In reply to comment #42) > I just discovered this issue here after downloading latest juno build and found > this huge thread about something that working perfectly fine before. It will be even more perfect with the new solution :) > From an eclipse user's perspective, this what I'd like to see eclipse do: > > 1. have an option to warn if a switch for an enum doesn't cover all cases. > 2. have an option to warn if other switch statements do not have a default case In the old approach you would not get any warnings regarding missing default cases. In M6 this warning conflicts with your item (1). With the new approach you can achieve this effect by: - enabling all options (see comment 45 or comment 11) - add default cases even to enum switches (perhaps empty) => You get warnings for both your items (1) and (2).
(In reply to comment #45) Thanks Stephan for the detailed summary. I like the approach you are taking. This should satisfy all the known requirements. Here are few comments/questions.. > Next I added the option to silence warnings by writing //$CASED-OMITTED$ before > the default case => no warnings re missing enum constant cases in this switch. Should not this be required even if default is not there? Some may want to intentionally handle only some enum constants and not others. > > Finally, I internally record this situation: > - all enum constants are covered AND > - missing default case is detected AND > - this warning is set to "ignore". > IF in this situation a flow-related error is reported for which the ignored > warning could be a valuable hint, THEN the flow problem is modified like so: > 769 = The local variable {0} may not have been initialized. Note that a problem Shouldn't all these errors really make sense only when all enum constants are 'not' covered. Otherwise, we shouldn't be reporting these errors in the first place. Isn't it? I glanced at the code but didn't start the code review. Do you want me to get started on the review?
(In reply to comment #45) Markus, Sergey, Dani, Others, do you have any comments on this Stephan's proposal? Please voice your concern/support if any. TIA.
Are the problems raised in comment 39 addressed in the new problem message 769-771? I think these new messages don't look absolutely necessary for now, so if there's a chance that they may be confusing in some situations, we should probably let them be, while keeping the new APIs at a minimum at this point in 3.8.
(In reply to comment #47) > (In reply to comment #45) > Thanks Stephan for the detailed summary. I like the approach you are taking. > This should satisfy all the known requirements. > > Here are few comments/questions.. > > Next I added the option to silence warnings by writing //$CASED-OMITTED$ before > > the default case => no warnings re missing enum constant cases in this switch. > Should not this be required even if default is not there? Some may want to > intentionally handle only some enum constants and not others. I'm fine with either way. If also a default is missing the problem is stronger so I thought we might want to keep reporting in that case. If majority thinks this tag should be independent of existence of a default case than I'll change the impl. accordingly. > > Finally, I internally record this situation: > > - all enum constants are covered AND > > - missing default case is detected AND > > - this warning is set to "ignore". > > IF in this situation a flow-related error is reported for which the ignored > > warning could be a valuable hint, THEN the flow problem is modified like so: > > 769 = The local variable {0} may not have been initialized. Note that a problem > Shouldn't all these errors really make sense only when all enum constants are > 'not' covered. Otherwise, we shouldn't be reporting these errors in the first > place. Isn't it? I'm not sure I understand the question: "all enum constants are 'not' covered" are you really speaking of a switch statement without any case statements? Or: "not all enum constants are covered"? Anyway: I'm talking indeed about the situation where the switch statement looks complete ("all enum constants are covered"), but still flow analysis sees a backdoor: via a enum constant added in the future. That's the case nobody understands right away. > I glanced at the code but didn't start the code review. Do you want me to get > started on the review? Except for the tiny issue in the Parser (regarding SwitchTest.test014/015), yes, code should be ready for review, if the concepts are approved.
(In reply to comment #49) > Are the problems raised in comment 39 addressed in the new problem message > 769-771? I tried my best both by replying to comment 39 and by improving the messages. > I think these new messages don't look absolutely necessary for now, I disagree, for me the hints in these messages are the only reason why I started working on all this. If we drop it lets drop it all. If you think it's confusing please make another proposal for addressing the issue.
(In reply to comment #50) > > Anyway: I'm talking indeed about the situation where the switch statement looks > complete ("all enum constants are covered"), but still flow analysis sees a > backdoor: via a enum constant added in the future. That's the case nobody > understands right away. I got it now. I didn't know that we are mandated to report the warning even when all the enum constants are covered. > > > I glanced at the code but didn't start the code review. Do you want me to get > > started on the review? > > Except for the tiny issue in the Parser (regarding SwitchTest.test014/015), > yes, code should be ready for review, if the concepts are approved. I will start the review.
(In reply to comment #51) > (In reply to comment #49) > > Are the problems raised in comment 39 addressed in the new problem message > > 769-771? > > I tried my best both by replying to comment 39 and by improving the messages. > > > I think these new messages don't look absolutely necessary for now, > > I disagree, for me the hints in these messages are the only reason why I > started working on all this. If we drop it lets drop it all. I don't think we should drop everything, but need to allay the concerns. Ayush's primary concern and actually mine too is the current state (M7) we are in. These messages are heuristics and may not be correct and yes, the messages are indicating that, but are we missing anything and if so, we may not have time to react. I don't see anything but I need to look at the code and play a little to get the confidence.
(In reply to comment #53) I also want to add that I think the hints are really useful. It is more of a timing thing.
General direction (keep 'warning' as default for incomplete switch and new missing default detection which is ignored by default) is looks good to me. What I don't like is the verbose problem message in case of missing default. We don't have to explain that. A user who sets the severity to 'warning' or error is already aware of this. A simple "missing default..." is good enough here. We e.g. also don't say "A ';' should be added to avoid a compile error". In the missing enum case, we could also improve the messages, e.g. if the switch is incomplete, one can either add the missing enum or add a default statement.
Created attachment 213904 [details] fix for regression in SwitchTest This tiny patch is to be applied on top of the other. It fixes the regression in SwitchTest by the following approach: when searching for special comments like $FALL-THROUGH$ - inspect the closest preceding comment - if it is not any special comment stop searching - if it starts with a '$' consider it as a special comment - if it matches the expected -> found - if it doesn't match -> keep searching backwards. So this special comment (from test014) is *not* accepted (no change): //$FALL-THROUGH$ - not last comment, thus inoperant // last comment is not fall-through explanation case 6://WRONG while here both special comments are recognized (new: EnumTest.test187): //$FALL-THROUGH$ //$CASES-OMITTED$ default : System.out.println("unknown"); break; I think this is the most straight-forward extension of the previous strategy.
Minor detail: COMPILER_PB_MISSING_DEFAULT_CASE should have "SWITCH" in it, e.g. COMPILER_PB_SWITCH_MISSING_DEFAULT_CASE
(In reply to comment #55) > What I don't like is the verbose problem message in case of missing default. We > don't have to explain that. I tried to accommodate Sergey's request in comment 38 (twisting it only slightly). I won't fight for this explanation part, though. > A user who sets the severity to 'warning' or error > is already aware of this. A simple "missing default..." is good enough here. To align with the other message would just this be fine: Missing default, depending on the type: 766 = The switch over the enum type {0} should have a default case 767 = The switch over the type {0} should have a default case ? > We e.g. also don't say "A ';' should be added to avoid a compile error". :) > In the missing enum case, we could also improve the messages, e.g. if the > switch is incomplete, one can either add the missing enum or add a default > statement. So, that's the part I didn't change at all. Note, that currently we raise one warning for each of the enum constants that is not covered. If warnings re missing default are enabled this are reported in addition, so this: enum Color { RED, GREEN, BLUE } switch (c) { case RED: break; } currently raises 3 problems: The enum constant GREEN needs a corresponding case label in this enum switch on Color The enum constant BLUE needs a corresponding case label in this enum switch on Color This enum switch should have a default case to account for future additions to the enum type Color Do you have any concerns with this? I like the clear separation of checking completeness re enum constant cases vs. the default case.
(In reply to comment #57) > Minor detail: COMPILER_PB_MISSING_DEFAULT_CASE should have "SWITCH" in it, e.g. > COMPILER_PB_SWITCH_MISSING_DEFAULT_CASE I agree. I did try to improve consistency of names, but unfortunately we are bound to controlling IProblem.MissingEnumConstantCase using JavaCore.COMPILER_PB_INCOMPLETE_ENUM_SWITCH, since both are established API.
Created attachment 213906 [details] UI fix assuming core constant is COMPILER_PB_SWITCH_MISSING_DEFAULT_CASE
> To align with the other message would just this be fine: > > Missing default, depending on the type: > 766 = The switch over the enum type {0} should have a default case > 767 = The switch over the type {0} should have a default case Yes. > The enum constant GREEN needs a corresponding case label in this enum > switch on Color ... > Do you have any concerns with this? I like the clear separation of checking > completeness re enum constant cases vs. the default case. I also like the separation as it is now - we should keep this. What I don't like is that it incompletely tries to tell me what to do. It should either say: "... needs a a corresponding case label in this enum switch on Color or a default case". or don't try to advise me what to do but instead simply tell me what's wrong.: "case GREEN is missing..."
Created attachment 213907 [details] combined patch For your convenience this patch combines my previous patches together with the renaming of the JavaCore constant. Currently rerunning all JDT/Core tests.
(In reply to comment #61) > ...What I don't > like is that it incompletely tries to tell me what to do. It should either say: > "... needs a a corresponding case label in this enum switch on Color or a > default case". > or don't try to advise me what to do but instead simply tell me what's wrong.: > "case GREEN is missing..." So you'd prefer these? 761 = A case label for the enum constant {1} is missing in this switch over the enum type {0} 766 = A default case is missing in this switch over the enum type {0} 767 = A default case is missing in this switch over the type {0} Two minor points against: - we can no longer differentiate "needs" vs. "should have" like I did to indicate that missing case is more severe if also a default is missing - I think the advice given by the messages in the current patch is in fact complete (if you read all the warnings) Anyway, nothing I'd fight for.
(In reply to comment #63) I definitely prefer messages from comment #45.
I see a funny "regression" in ASTConverter15Test.test0151(): Unexpected errors: 1. WARNING in /Converter15/src/X.java (at line 5)\n @SuppressWarnings("incomplete-switch")\n ^^^^^^^^^^^^^^^^^^^\n Unnecessary @SuppressWarnings("incomplete-switch")\n In fact this @SW was *never* necessary. But somehow by enabling this warning per default we only now start reporting that the @SW is unnecessary. I haven't seen this during previous test runs. It would be trivial to fix the failure, but I'm puzzled why this occurs only now. All other JDT/Core tests are happy. Only: looking at this issue reminded me, that IrritantSet is missing a line like this: INCOMPLETE_SWITCH.set(CompilerOptions.MissingDefaultCase); This is needed so the new warnings can also be suppressed using the token "incomplete-switch". Finally, the batch compiler uses an option called "enumSwitch", which is slightly off when we warn about a missing default in a non-enum switch :(
Created attachment 213940 [details] UI patch Here's the complete UI patch for the preference page (including the "even if default case exists" option). Quick fixes are not ready yet. The API and behavior of the "combined patch" looks good. I didn't look at the SuppressWarnings though. Just wanted to point to the related bug 376090. Re //$CASES-OMITTED$: For the similar fall-through problem, the problem message mentions the silencer: 194 = Switch case may be entered by falling through previous case. If intended, add a new comment //$FALL-THROUGH$ on the line above I think message 768 should also mention the magic comment, e.g.: 768 = The enum constant {1} should have a corresponding case label in this enum switch on {0}. To suppress this problem, add a comment //$CASES-OMITTED$ on the line above the 'default:'
> Re //$CASES-OMITTED$: > For the similar fall-through problem, the problem message mentions the > silencer: > 194 = Switch case may be entered by falling through previous case. If intended, > add a new comment //$FALL-THROUGH$ on the line above > > I think message 768 should also mention the magic comment, e.g.: > 768 = The enum constant {1} should have a corresponding case label in this > enum > switch on {0}. To suppress this problem, add a comment //$CASES-OMITTED$ on the > line above the 'default:' In that case I'd rather vote to remove the hint from 194 because we generally don't put the fix hint into the problem message. And 'yes', we also have to add a quick fix which add "//$CASES-OMITTED$".
> So you'd prefer these? > > 761 = A case label for the enum constant {1} is missing in this switch over > the > enum type {0} > > 766 = A default case is missing in this switch over the enum type {0} > 767 = A default case is missing in this switch over the type {0} I was not against using "should" and "needs". Only against the long explanation/hint and I raised an unrelated topic where in case of missing enum case, we could not only say "needs a corresponding case label" but "needs a corresponding case label or default case". But I think we could discuss that in a separate bug and deal with it later.
(In reply to comment #67) > In that case I'd rather vote to remove the hint from 194 because we generally > don't put the fix hint into the problem message. Generally, yes. Generally, a user can consult the language specification for possible fixes. But I'd consider these two examples exceptions, since these are special features that are only available in our compiler, and there is no obvious fix.
(In reply to comment #62) This patch looks good for me. There is one todo to change the MethodScope#hasMissingSwitchDefault. There is one minor improvement that is possible in Parser#hasLeadingTagComment(). The 'if (iTag == 0)' could be moved out into the outer loop. I liked the messages proposed in comment 58 and comment 66.
In general +1. The messages are details which we can still fine tune.
(In reply to comment #71) > In general +1. The messages are details which we can still fine tune. Please make sure to post the API change request on the PMC mailing list with explanation why this is needed.
(In reply to comment #70) > (In reply to comment #62) > This patch looks good for me. There is one todo to change the > MethodScope#hasMissingSwitchDefault. If you agree I'll open a new bug to collate several of the boolean flags into one int. Didn't want to do this refactoring at this time, though, because it could possibly spread into many locations. > There is one minor improvement that is > possible in Parser#hasLeadingTagComment(). The 'if (iTag == 0)' could be moved > out into the outer loop. I'm not sure what you mean. If we are looking at the same location the enclosing loop is for (int iTag = 0, length = commentPrefixTag.length; iTag < length; iTag++, charPos++) Moving (iTag == 0) outside that loop won't work :)
I've pushed the UI updates (preferences and quick fixes) to branch http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/log/?h=mkeller/Bug_374605_Unreasonable_warning_for_enum-based_switch_statements
Created attachment 214014 [details] Minor updates > (In reply to comment #62) > I liked the messages proposed in comment 58 and comment 66. By some notion of majority vote these seem acceptable -> I updated the messages with these proposals. (In reply to comment #65) > I see a funny "regression" in ASTConverter15Test.test0151(): Mystery solved: all can be explained by how defaults affect this test. Fixed by reverting to the state prior to bug 265744. > looking at this issue reminded me, that IrritantSet is missing a line like this: > > INCOMPLETE_SWITCH.set(CompilerOptions.MissingDefaultCase); Done. > Finally, the batch compiler uses an option called "enumSwitch", which is > slightly off when we warn about a missing default in a non-enum switch :( Not done. Any ideas? Postpone? Current effect is to control all problems that are also controlled by @SW("incomplete-switch"), which isn't 100% correct.
(In reply to comment #73) > If you agree I'll open a new bug to collate several of the boolean flags into > one int. Didn't want to do this refactoring at this time, though, because it > could possibly spread into many locations. For curiousity sake, can't we move this into the ASTNode tag bits of MethodDeclaration itself? Sure, moving all into flags could be nice and could be handled separately. > Moving (iTag == 0) outside that loop won't work :) Doesn't the following code doesn't work? Am I missing anything? #### if(source[charPos++] != commentPrefixTag[0]) return false; for (int iTag = 1, length = commentPrefixTag.length; iTag < length; iTag++, charPos++) { if (charPos >= rangeEnd) return false; // comment is too small to host tag if (source[charPos] != commentPrefixTag[iTag]) continue previousComment; } return true; ####
(In reply to comment #76) > (In reply to comment #73) > > If you agree I'll open a new bug to collate several of the boolean flags into > > one int. Didn't want to do this refactoring at this time, though, because it > > could possibly spread into many locations. > For curiousity sake, can't we move this into the ASTNode tag bits of > MethodDeclaration itself? Sure, moving all into flags could be nice and could > be handled separately. several minor concerns here: - we are running out of bits in tagBits - MethodDeclaration is one hop further away - MethodScope already wastes some memory with individual boolean fields, which could be reduced with just this one refactoring. If the refactoring doesn't cause any trouble down the road I'd stick to the idea to just combine several booleans in MethodScope into one bitset. > > Moving (iTag == 0) outside that loop won't work :) > Doesn't the following code doesn't work? Am I missing anything? > #### > if(source[charPos++] != commentPrefixTag[0]) return false; > for (int iTag = 1, length = commentPrefixTag.length; iTag < length; iTag++, > charPos++) { > if (charPos >= rangeEnd) return false; // comment is too small to host tag > if (source[charPos] != commentPrefixTag[iTag]) continue previousComment; > } > return true; > #### If you add the required checks to prevent AIOOBE this will work, but then it will no longer be simpler than the current solution, because these checks are then duplicate. :)
(In reply to comment #77) > several minor concerns here: > - we are running out of bits in tagBits > - MethodDeclaration is one hop further away > - MethodScope already wastes some memory with individual boolean fields, which > could be reduced with just this one refactoring. > If the refactoring doesn't cause any trouble down the road I'd stick to the > idea to just combine several booleans in MethodScope into one bitset. Agreed. > If you add the required checks to prevent AIOOBE this will work, but then it > will no longer be simpler than the current solution, because these checks are > then duplicate. :) OK, I thought I was missing something else. Preventing AIOOBE is not hard without any duplicate checks too. The following should do. ### if(charPos + commentPrefixTag.length >= rangeEnd) continue previousComment; if (source[charPos++] != commentPrefixTag[0]) return false; for (int iTag = 1, length = commentPrefixTag.length; iTag < length; iTag++, charPos++) { if (source[charPos] != commentPrefixTag[iTag]) continue previousComment; } return true; ### I am not particular with the change but I think this code is better. BTW, I now see one problem - I think the line --- if (source[charPos] != commentPrefixTag[iTag]) return false; should be --- if (source[charPos] != commentPrefixTag[iTag]) continue previousComment;
(In reply to comment #75) > > Finally, the batch compiler uses an option called "enumSwitch", which is > > slightly off when we warn about a missing default in a non-enum switch :( > > Not done. Any ideas? Postpone? > Current effect is to control all problems that are also controlled by > @SW("incomplete-switch"), which isn't 100% correct. We can perhaps add a master option to control switch related warnings and then add sub-options to it, like we did earlier for null annotations, etc. Shouldn't take a lot of time. What do you think?
(In reply to comment #78) Frankly, I don't see which problem we are trying to solve in the Parser change. My story was: keep the entire logic of matching tag comments as it was, only when we find a mismatch decide: - didn't match any chars -> give up, this was not a tag comment - matched at least the initial '$' -> keep looking through previous comments That's exactly what the code said, too. I don't see this getting clearer with the proposed changes, so perhaps you see a different story of what actually this snippet implemenents? Are you optimizing for performance or for readability?
(In reply to comment #79) > (In reply to comment #75) > > > Finally, the batch compiler uses an option called "enumSwitch", which is > > > slightly off when we warn about a missing default in a non-enum switch :( > > > > Not done. Any ideas? Postpone? > > Current effect is to control all problems that are also controlled by > > @SW("incomplete-switch"), which isn't 100% correct. > > We can perhaps add a master option to control switch related warnings and then > add sub-options to it, like we did earlier for null annotations, etc. Shouldn't > take a lot of time. What do you think? That would take care of everything. Not 100% sure we need this level of granularity at the CLI. My concern, however, is regarding the existing option which is published API and thus cannot be renamed. Even if we add your proposal we would have to maintain the old "enumSwitch" option, no? What bugs me is the inconsistency between the two existing tokens: "incomplete-switch" (@SW) and "enumSwitch" (CLI), which were intended to say the same, but with different words. If we leave it at just one batch compiler option, what would be better acceptable: (a) the option also controls missing default in non-enum switch although it says "enumSwitch" (b) missing default in non-enum switch cannot be controlled at the CLI (always off) ? (b) would actually require one more IProblem, to distinguish. Would be trivial to implement, though.
(In reply to comment #80) They are two issues. The one that I was discussing with you in the optimization for performance. I try to pull out 'if (index == 0)' checks out of a loop for performance. I reiterate that I am not particular with this change. The second issue is there is a bug in the function just above the code you had modified. This will not surface right now, but better to fix it. You had 'rightly' modified the code to continue the loop, if the 'second' char doesn't match. However, the line above this change which checks for rangeEnd should also be modified to continue the loop rather than return. This is necessary because the comments could be of different length. Currently, the tags length differ by only one and hence we will run into an issue now, but if we add more tags, we could run. Hence, we should fix this. --- if (source[charPos] != commentPrefixTag[iTag]) return false; should be +++ if (source[charPos] != commentPrefixTag[iTag]) continue previousComment;
API change approval request from Satyam: http://dev.eclipse.org/mhonarc/lists/eclipse-pmc/msg01574.html +1 from me, but note that the PMC vote lasts 48 hours.
(In reply to comment #81) > "incomplete-switch" (@SW) and "enumSwitch" (CLI), which were intended to say > the same, but with different words. The options should be split up anyway, see bug 375409. Then we can keep "enumSwitch" (CLI) with its old meaning and add 2 new tokens for the new options. BTW: Main#handleErrorOrWarningToken(..) already tried to support "incomplete-switch", but I think that never worked because it's in the "case 'e':" group.
Created attachment 214230 [details] release candidate After we got PMC approval here's a release candidate patch (only minor, non-API changes): Follow hint in comment 82 regarding different tag lengths (Parser). Split CLI options following comment 84: - enumSwitch: old option reverted to old semantics - enumSwitchPedantic + report missing enum switch cases even in the presence of a default case - switchDefault + switch statement lacking a default case "enumSwitchPedantic" is the shortest transcription I could find for ReportMissingEnumCaseDespiteDefault. I implemented the following dependency: when enabling "enumSwitchPedantic" check severity of "enumSwitch", if that is less then the currently requested severity, increase it, e.g,. -err:+enumSwitchPedantic will set severity for OPTION_ReportIncompleteEnumSwitch to error. However, the -pedantic option will never decrease this severity. Currently doing a final run of all JDT/Core tests.
Released (incl. copyright updates) for 3.8 M7 via commit 44ff943ce2a18d1de59c739946fda0722d1ad727
Updated preferences UI and quick fixes. http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=9f976e314e5e889647f3ec56243faf2155a88f06
I've also fixed the UI tests I forgot, and I added double quotes in jdt.core's messages.properties (to satisfy CHKPII).
Verified for 3.8M7 using build I20120429-1800
