Bugzilla – Full Text Bug Listing
| Summary: | [quick assist] Provide a "Convert 'if-else' to 'switch'" quick assist | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||
| Component: | UI | Assignee: | Deepak Azad <deepakazad> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | billc.cn, daniel_megert, markus.kell.r, pbenedict, raksha.vasisht | ||||
| Version: | 3.7 | ||||||
| Target Milestone: | 4.3 M1 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Deepak Azad
When all the if-expressions are regular "==" checks of the form
"if (key.equals(<constantString>)" for String keys of "if (key == 1)" or
"if (1 == key)", then we don't have any issues with NPEs, not even if "key" has a boxed type.
For "if (<constantString>.equals(key))", the behavior-preserving transformation is to execute the default statements. If they are not null, an extra
"if (key ==null) { <defaultStatements> } else { switch ... }" is necessary (thereby duplicating the default statements).
Created attachment 199573 [details]
Patch_v0.5
Patch under construction.
1) Conversion if the if-else block contains one or more expressions with either "equals" or "==" , one of the operand being a constant value or a String Literal
2) Conversion to switch with Enum constants with Qualified Names
Ex that does not work :
if (TEST.equals(s) || "TEST1".equals(s) || "TEST3".equals(s) || "TEST4".equals(s)) {
System.out.println("test1"); //$NON-NLS-1$
} else {
System.out.println("default"); //$NON-NLS-1$
}
Need to add support to handle additional operands list in the Infix Expressions.
Released the strings part to BETA_JAVA7.
Ping! Will this make it to 3.7.1? We need the N&N by next week. We'll move this to 3.8. Time permitting for 3.8. Due to all the new switch warnings added in 3.8, please make sure the outputted switch won't generate those warnings. For example, always include a default: label? Fixed via http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=fe25d5809f1fc0820bacaf9c225cfa7f04f0c8b0 (In reply to comment #1) > For "if (<constantString>.equals(key))", the behavior-preserving transformation > is to execute the default statements. If they are not null, an extra > "if (key ==null) { <defaultStatements> } else { switch ... }" is necessary > (thereby duplicating the default statements). Oops, I missed this case. Will fix it. (In reply to comment #6) > Due to all the new switch warnings added in 3.8, please make sure the outputted > switch won't generate those warnings. For example, always include a default: > label? The default settings/behavior has not changed, so I have not done anything special so far. However, we could probably read the compiler preferences and generate code such that there are no warnings and that all warnings that should be there are there. E.g. unless 'signal even if default case exists' is enabled we should not generate default cases since that would suppress potential 'Incomplete switch cases on enum' warnings. Deepak, I noticed all the test cases follow if..else if..else pattern, but none are if..else if without else. In such a situation, will a default: case be generated? And should there be a test for that? (In reply to comment #9) > Deepak, I noticed all the test cases follow if..else if..else pattern, but none > are if..else if without else. In such a situation, will a default: case be > generated? Nope. > And should there be a test for that? I will add a test when I fix the remaining issues. (In reply to comment #8) > E.g. unless 'signal even if default case exists' is enabled > we should not generate default cases since that would suppress potential > 'Incomplete switch cases on enum' warnings. Just to be 100% clear, what I meant was if the original if-else construct does not contain a 'default' case, the resultant switch statement will also not have a 'default' case. There is no way to completely preserve behavior in snippets where 'one style' w.r.t to <const>.equals(<var>) is not followed consistently.
----------------------------------------
void foo3(String s) {
if ("aa".equals(s)) {
} else if (s.equals("bb")) {
} else if ("cc".equals(s)) {
}
}
---------------------------------------
Hence, the strategy that I am inclined to follow is that if "if (<constantString>.equals(key))" is encountered even once the result will be of the form "if (key ==null) { <defaultStatements> } else { switch ... }" since (I think) that's what most developers would want.
(In reply to comment #11) That's OK for me. We could also just not offer the quick assist if the if-else construct is not completely regular. It might be beneficial to provide two quick assists if the switch is working on an String/Enum. One that preserves throwing NPE and another that just ignores it.
switch (s) {
case "a":
case "b":
}
* Convert 'switch' to 'if-else'
if ("a".equals(s)) {
...
} else if ("b".equals(s)) {
...
}
* Convert 'switch' to 'if-else' with null pointer check
if (s == null) {
throw new NullPointerException();
} else if ("a".equals(s)) {
...
} else if ("b".equals(s)) {
...
}
(In reply to comment #13) This bug is not about "Convert 'switch' to 'if-else'". Please file a new bug if you think that "Convert 'switch' to 'if-else' with null pointer check" would really be useful. Fixed remaining issues and added necessary tests http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8cd12906e44758cb73ff76b6b70f3eca52f691d3 (In reply to comment #12) > (In reply to comment #11) > That's OK for me. We could also just not offer the quick assist if the if-else > construct is not completely regular. I offer the quick assist, since the irregularity is probably a silly error on part of the developer and not offering the quick assist would come in the way. (In reply to comment #13) > It might be beneficial to provide two quick assists if the switch is working on > an String/Enum. One that preserves throwing NPE and another that just ignores > it. Here we are converting if-else-if to switch :-) In any case, I think we should offer one quick assist which just does the 'right' thing. Marking this bug as Fixed. I filed bug 383110 for problems with switch on non-primitive types. Thanks Markus. My comment 13, I think, alludes to your desire to make the quick fixes inverses of each other. Glad you picked that out and clarified the boundary conditions. (In reply to comment #16) > I filed bug 383110 for problems with switch on non-primitive types. Right, so why is this bug reopened? Are there more issues? > Right, so why is this bug reopened? Are there more issues?
Sorry, it's getting late :-/. I first wanted to reopen this bug, then decided to open a new one, and then added my comment in the tainted browser tab.
(In reply to comment #19) > Sorry, it's getting late :-/. I first wanted to reopen this bug, then decided > to open a new one, and then added my comment in the tainted browser tab. np :-) Thanks for the reviewing the feature! Verified in I20120806-2000. |