Bugzilla – Full Text Bug Listing
| Summary: | Adding elements to an enum body with trailing comma generates bad code | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Chris West (Faux) <eclipse> | ||||
| Component: | Core | Assignee: | Olivier Thomann <Olivier_Thomann> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | jarthana, markus.kell.r, Olivier_Thomann, satyam.kandula, srikanth_sankaran | ||||
| Version: | 3.7 | Flags: | srikanth_sankaran:
review+
|
||||
| Target Milestone: | 3.6.1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows Server 2003 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Self-contained example:
package xy;
public class From {
// in Outline, drag a to Scratch:
int a;
}
enum Scratch {
A, B,
}
Bug in ASTRewrite. Works fine if there's no "," after "A, B".
Created attachment 172773 [details]
Proposed fix + regression tests
Since this is producing bad code, I think it could be a candidate for 3.6.1. Srikanth, please review. Released for 3.7M1. Will close as FIXED once it is reviewed for 3.6.1. (In reply to comment #3) > Since this is producing bad code, I think it could be a candidate for 3.6.1. > Srikanth, please review. I am not familiar with this code, I stepped through the changes and the patch looks reasonable. Did you intentionally leave in the commented line of code that looks like: // doTextReplace(pos, endPos - pos, ";", getEditGroup(children[0])); //$NON-NLS-1$ (In reply to comment #5) > I am not familiar with this code, I stepped through the changes and the > patch looks reasonable. Did you intentionally leave in the commented > line of code that looks like: > // doTextReplace(pos, endPos - pos, ";", getEditGroup(children[0])); > //$NON-NLS-1$ I'll get rid of the comment. Released in 3.6 maintenance. This now (N20100703-2000) generates:
enum A {
A,
B,;
int a = 5;
}
While the ,; is correct and legal (as far as I'm aware), I doubt it's what most people would expect? The person who wrote the unit tests obviously disagrees.
I also wouldn't expect a user to write ...
enum Scratch { A, B, }
... in the first place. But if he does, AST rewrites should not do any magic to existing code that doesn't need any changes for a correct rewrite.
"Remove unnecessary trailing commas" could be a new clean up, though (very low priority for me).
(In reply to comment #8) > enum A { > A, > B,; > > int a = 5; > } > > While the ,; is correct and legal (as far as I'm aware), I doubt it's what most > people would expect? The person who wrote the unit tests obviously disagrees. The rewrite support should not try to cleanup user's style. If there is an extra comma, it must be preserved. As Markus suggested, we could add an extra clean up action to get rid of it. Verified for 3.7M1 using build I20100802-1800. Keeping the RESOLVED STATUS as this needs to be verified for 3.6.1 Verified for 3.6.1 RC2 using Build id: M20100825-0800 |
Build Identifier: M20100211-1343 If you have a (perfectly legal and sensible) enum like: enum Scratch { A, B, } ...dragging an existing field into it from another type will generate: enum Scratch { A, B; int a;, } ...where the comma after the ; is (obviously) illegal. API consumer POV: AbstractTypeDeclaration a; a.bodyDeclarations().add(ast.newFieldDeclaration( etc. --> badness. Also tested on 3.6RC4. Reproducible: Always Steps to Reproduce: 1. Create a class containing just enum Scratch { A, B, } 2. Create another class with a single int field in it: class Q { int a; } 3. Expand both in the package explorer and drag "a" from "Q" into "Scratch".