Page MenuHomePhabricator

Ignore trailing NullStmts in StmtExprs for GCC compatibility
Needs ReviewPublic

Authored by domdom on Jan 22 2019, 10:50 PM.

Details

Summary

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.

Diff Detail

Event Timeline

domdom created this revision.Jan 22 2019, 10:50 PM
domdom updated this revision to Diff 183043.Jan 22 2019, 11:32 PM

Formatting

aaron.ballman edited reviewers, added: aaron.ballman; removed: lattner.Jan 23 2019, 6:17 AM
aaron.ballman added a subscriber: aaron.ballman.

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
962–971

gcc -> GCC

Also, add a full stop to the end of the comment.

963

This should be unsigned rather than int.

965

Prefer ++LookHead; since the result isn't needed.

971

Remove spurious newline.

clang/lib/Sema/SemaExpr.cpp
13324

Add full stop to the end of the sentence.

13328

Add full stop to the end of the sentence.

13331

Add full stop to the end of the sentence.

domdom updated this revision to Diff 184001.Jan 28 2019, 5:44 PM

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.

domdom marked 7 inline comments as done.Jan 28 2019, 5:46 PM
aaron.ballman added inline comments.Jan 30 2019, 5:51 AM
clang/include/clang/AST/Stmt.h
1261

How about getIndexOfLastNonNullStmt() since it doesn't return the Stmt* itself?

clang/lib/CodeGen/CGStmt.cpp
385–387

const Stmt *

392

What happens if ExprResult is nullptr?

clang/test/CodeGen/exprs.c
202

Does this test need the extra null statement between the 3; and 4;?

domdom updated this revision to Diff 184413.Jan 30 2019, 5:01 PM
domdom updated this revision to Diff 184414.
domdom marked 6 inline comments as done.
domdom marked an inline comment as not done.Jan 30 2019, 5:05 PM
domdom added inline comments.
clang/include/clang/AST/Stmt.h
1261

Agreed.

1305

getLastNonNullStmt asserts anyway, should I remove this?

clang/lib/CodeGen/CGStmt.cpp
392

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.

aaron.ballman added inline comments.Jan 31 2019, 10:47 AM
clang/include/clang/AST/Stmt.h
1261

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.

1305

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.

1314

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
392

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!

domdom updated this revision to Diff 185196.Feb 4 2019, 5:13 PM

Switched getIndexOfLastNonNullStmt to return Optional<unsigned> instead of Stmt* as per comments.

domdom marked 6 inline comments as done.Feb 4 2019, 5:14 PM
aaron.ballman added inline comments.Feb 5 2019, 7:48 AM
clang/include/clang/AST/Stmt.h
1259–1260

Given the name of the function, why return the index of the last null statement if it only contains null statements?

1270

The only way you can get here is if all statements are null statements, so this should return None as well, shouldn't it?

1315–1316

return ExprResult ? body_begin()[*ExprResult] : nullptr;

riccibruno added inline comments.
clang/include/clang/AST/Stmt.h
1260

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.

1303

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 ?

1312

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.)