Ignore trailing NullStmts in compound expressions when determining the result type and value. This is to match the GCC behavior which ignores semicolons at the end of compound expressions.
Details
Diff Detail
Event Timeline
Can you also add a test case to TestMiscStmts() in clang\test\AST\ast-dump-stmt.c that demonstrates we calculate the correct resulting type and don't lose the extra null statements from the AST?
clang/lib/Parse/ParseStmt.cpp | ||
---|---|---|
975–984 | gcc -> GCC Also, add a full stop to the end of the comment. | |
976 | This should be unsigned rather than int. | |
978 | Prefer ++LookHead; since the result isn't needed. | |
984 | Remove spurious newline. | |
clang/lib/Sema/SemaExpr.cpp | ||
13372 | Add full stop to the end of the sentence. | |
13374–13376 | Add full stop to the end of the sentence. | |
13377 | Add full stop to the end of the sentence. |
Thanks for your comments, @aaron.ballman. I've addressed the comments and added a test case as suggested.
This revealed an issue with the code-gen side of things, so I fixed that and added another test case for that as well.
clang/include/clang/AST/Stmt.h | ||
---|---|---|
1303 | Agreed. | |
1328 | getLastNonNullStmt asserts anyway, should I remove this? | |
clang/lib/CodeGen/CGStmt.cpp | ||
412 | Then ExprResult == *I should not be evaluated. (Since GetLast would be false) | |
clang/test/CodeGen/exprs.c | ||
202 | Not strictly speaking, no. Just added it to ensure it has no effect. |
clang/include/clang/AST/Stmt.h | ||
---|---|---|
1303 | What do you think about having this function return an optional<unsigned> rather than asserting? This will keep the API safe when called on an empty CompoundStmt. | |
1328 | If you did switch to using an optional, then you can assert the optional has a value here. The effect should be the same as your current approach, but with slightly more tolerance. | |
1337 | And if getLastIndexOfNonNullStmt() returns an empty value here, you can return nullptr to signify that there is no statement expression result possible. | |
clang/lib/CodeGen/CGStmt.cpp | ||
412 | I think I was worried about a different situation that isn't present in this patch. However, if you switch to using optional, then getStmtExprResult() could potentially return null as well (when GetLast is true), so we'd have to handle that (probably with an assertion). | |
clang/test/CodeGen/exprs.c | ||
202 | That sounds good to me! |
Switched getIndexOfLastNonNullStmt to return Optional<unsigned> instead of Stmt* as per comments.
clang/include/clang/AST/Stmt.h | ||
---|---|---|
1301–1302 | Given the name of the function, why return the index of the last null statement if it only contains null statements? | |
1312 | The only way you can get here is if all statements are null statements, so this should return None as well, shouldn't it? | |
1338–1339 | return ExprResult ? body_begin()[*ExprResult] : nullptr; |
clang/include/clang/AST/Stmt.h | ||
---|---|---|
1302 | Additionally I am not sure that the comment is optimal. This is inside CompoundStmt and it is therefore strange to use "if this compound expression", since the compound expression is represented with the StmtExpr node. | |
1326 | I think it needs at the very least to mention that this is about the GNU extension. Perhaps it would be useful to mention the relation between the CompoundStmt node and the StmtExpr node ? Also more generally is it not possible to avoid mutating the compound statement node after it has been created ? | |
1335 | Same, I would find it clearer if you mentioned the extension. |
You'll also need to update TreeTransform::TransformCompoundStmt. (And please add some tests for template instantiation.)
Sorry I it's taken me a while to get back to this work. I've rebased the changes and taken advantage of the refactoring to stop modifying the CompoundStmt after creating it. This definitely simplified the changes required in Stmt.h, which is nice.
I've addressed the the need to update the TreeTransformer for the template case, and added a test case for that.
Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?
Thanks! I think this is looking good to me, but you should wait for the other reviewers before committing in case they have further concerns.
Something I should ask, it seems like GCC only ignores the NullStmts at the end if it's in C mode. Should clang match this behaviour exactly?
I can't think of a reason that this should only happen in C mode, can you @rsmith?
clang/test/SemaCXX/statements.cpp | ||
---|---|---|
41–52 | Be sure to run the patch through clang-format. |
No, I can't think of such a reason either. When we've seen such weirdnesses before (eg, break or continue in a statement expression in a for loop increment expression) we generally try to pick a behavior that's consistent across languages, and warn on the cases where we give a different behavior from GCC as a result.
You are very welcome; thank you both for your comments!
I do need someone to commit on my behalf :)
I'm sorry for the incredibly long delay in committing this for you -- I managed to lose track of this thread. I went to apply the changes today and get the following test failures when trying on Windows 10 x64:
FAIL: Clang :: AST/ast-dump-stmt.c (152 of 11055) ******************** TEST 'Clang :: AST/ast-dump-stmt.c' FAILED ******************** Script: -- : 'RUN: at line 1'; c:\cmakebuilds\build\x64-debug\bin\clang.exe -cc1 -internal-isystem c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include -nostdsysteminc -std=gnu11 -ast-dump C:\llvm\tools\clang\test\AST\ast-dump-stmt.c | c:\cmakebuilds\build\x64-debug\bin\filecheck.exe -strict-whitespace C:\llvm\tools\clang\test\AST\ast-dump-stmt.c -- Exit Code: 1 Command Output (stdout): -- $ ":" "RUN: at line 1" $ "c:\cmakebuilds\build\x64-debug\bin\clang.exe" "-cc1" "-internal-isystem" "c:\cmakebuilds\build\x64-debug\lib\clang\9.0.0\include" "-nostdsysteminc" "-std=gnu11" "-ast-dump" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c" # command stderr: C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:48:3: warning: expression result unused -T1; ^~~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:49:3: warning: expression result unused -T2; ^~~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:58:3: warning: expression result unused ~T1; ^~~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:59:3: warning: expression result unused ~T2; ^~~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:70:21: warning: expression result unused _Generic(i, int : 12); ^~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:79:21: warning: expression result unused _Generic(i, int : 12, default : 0); ^~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:90:34: warning: expression result unused _Generic(i, default : 0, int : 12); ^~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:101:21: warning: expression result unused _Generic(i, int : 12, float : 10, default : 100); ^~ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:142:3: warning: expression result unused 0; ^ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:8: warning: expression result unused for (b; b; b) ^ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:288:14: warning: expression result unused for (b; b; b) ^ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:367:17: warning: expression result unused ({int a = 10; a;}); ^ C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:375:3: warning: expression result unused ({int a = 10; a;;; }); ^~~~~~~~~~~~~~~~~~~~~ 13 warnings generated. $ "c:\cmakebuilds\build\x64-debug\bin\filecheck.exe" "-strict-whitespace" "C:\llvm\tools\clang\test\AST\ast-dump-stmt.c" # command stderr: C:\llvm\tools\clang\test\AST\ast-dump-stmt.c:376:18: error: CHECK-NEXT: expected string not found in input // CHECK-NEXT: StmtExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:22> 'int' ^ <stdin>:282:5: note: scanning from here `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int' ^ <stdin>:282:5: note: with "@LINE-1" equal to "375" `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int' ^ <stdin>:282:14: note: possible intended match here `-StmtExpr 0x19f2d493d18 <line:375:3, col:23> 'int' ^ error: command failed with exit status: 1 --
It looks like the column number is off by one, and I wasn't certain why. Can you look into that before I commit?
Thank you for catching that @aaron.ballman!
I forgot to update the test after the running clang-format. I thought I ran the tests again before submitting, but I guess I didn't :P
Thanks again!
Given the name of the function, why return the index of the last null statement if it only contains null statements?