Bugzilla – Full Text Bug Listing
| Summary: | Invalid AST produced by the parser | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Sergey Prigogin <eclipse.sprigogin> |
| Component: | cdt-parser | Assignee: | Project Inbox <cdt-parser-inbox> |
| Status: | REOPENED --- | QA Contact: | Jonah Graham <jonah> |
| Severity: | normal | ||
| Priority: | P3 | CC: | cdtdoug, malaperle, yevshif, zeratul976 |
| Version: | 8.0 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
Similar to bug 105334. I will follow the same strategy as used for the fix of bug 105334, i.e. an artificial compound-statement is inserted into the AST. The more correct fix would make the statement following the case/default label part of the IASTCaseStatement/IASTDefaultStatement. However, I don't think we should make this kind of change after the API has been frozen. Do you agree with this solution? For reference, here is the relevant part of the grammar: labeled-statement: attribute-specifier-seqopt identifier : statement attribute-specifier-seqopt case constant-expression : statement attribute-specifier-seqopt default : statement Fixed in 8.0 > 20110421. *** cdt cvs genie on behalf of mschorn *** Bug 343479: Switch statement without compound statement. [*] AbstractGNUSourceCodeParser.java 1.162 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/AbstractGNUSourceCodeParser.java?root=Tools_Project&r1=1.161&r2=1.162 (In reply to comment #1) > Similar to bug 105334. I will follow the same strategy as used for the fix of > bug 105334, i.e. an artificial compound-statement is inserted into the AST. This approach is problematic since it is likely to confuse code formatter. Code formatter will expect curly braces that are not present in the code. In fact, the AST anomaly was discovered while debugging code formatter. I would like to add that the issue seems serious enough to warrant an exemption from the API freeze. ICPPASTIfStatement didn't get reparented as a result of the parser change. The new structure is:
ICPPASTTranslationUnit
ICPPASTFunctionDefinition: test
ICPPASTSimpleDeclSpecifier: void
ICPPASTFunctionDeclarator
IASTName: test
IASTCompoundStatement
ICPPASTSwitchStatement
ICPPASTLiteralExpression: 0
IASTCompoundStatement
IASTDefaultStatement
ICPPASTIfStatement
ICPPASTLiteralExpression: true
IASTNullStatement
I'm going to revert the change.
*** cdt cvs genie on behalf of sprigogin *** Bug 343479. Reverted previous change. [*] AbstractGNUSourceCodeParser.java 1.163 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/AbstractGNUSourceCodeParser.java?root=Tools_Project&r1=1.162&r2=1.163 *** cdt cvs genie on behalf of mschorn *** Bug 343479: Switch statement without compound statement. [*] AbstractGNUSourceCodeParser.java 1.164 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/AbstractGNUSourceCodeParser.java?root=Tools_Project&r1=1.163&r2=1.164 (In reply to comment #6) Thx, I have completed the change. With that we have at least the same solution for 'switch(cond) case ...' and 'switch(cond) default ...'. (In reply to comment #5) > I would like to add that the issue seems serious enough to warrant an > exemption from the API freeze. I would not worry a lot if we'd change the AST for the special situation, only. However, a correct modelling requires to nest subsequent case label statements into each other and this change has the potential to break clients (e.g. flow analysis? formatter?). I am conservative about it and will make the change only after 8.0, however you and the other committers can make a different decision. switch(x) { case 1: case 2: x= 2; break; } Current AST: switch-stmt compound-stmt case-stmt case-stmt assignment break-stmt Future AST: switch-stmt compound-stmt case-stmt case-stmt assignment break-stmt (In reply to comment #9) Thanks for the fix. It was sufficient for the code formatter. We should keep the bug open and rearrange the AST after 8.0. (In reply to Markus Schorn from comment #9) > switch(x) { > case 1: > case 2: > x= 2; > break; > } > > Current AST: > switch-stmt > compound-stmt > case-stmt > case-stmt > assignment > break-stmt > > Future AST: > switch-stmt > compound-stmt > case-stmt > case-stmt > assignment > break-stmt Is there a reason the AST cannot be switch-stmt compound-stmt case-stmt case-stmt assignment break-stmt Here a case-stmt would act as a compound statement, with 0 or more statement children. I think this structure reflects the code most closely, and is intuitive for things like the formatter to deal with. |
The following code void test() { switch (0) default: if (true) ; } gets parsed into: ICPPASTTranslationUnit ICPPASTFunctionDefinition: test ICPPASTSimpleDeclSpecifier: void ICPPASTFunctionDeclarator IASTName: test IASTCompoundStatement ICPPASTSwitchStatement ICPPASTLiteralExpression: 0 IASTDefaultStatement ICPPASTIfStatement ICPPASTLiteralExpression: true IASTNullStatement ICPPASTIfStatement, which should be a child of IASTDefaultStatement, is instead considered a child of the function body. The code in this example may look exotic, but it is similar to the code used inside googletest (http://code.google.com/p/googletest) assertions.