[PATCH] D57086: Ignore trailing NullStmts in StmtExprs for GCC compatibility
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 31 10:47:36 PST 2019
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Stmt.h:1300
+ void setStmtExpr(Stmt *S) {
+ assert(!body_empty() && "setStmtExpr");
+ unsigned ExprResult = getLastNonNullStmt();
----------------
domdom wrote:
> getLastNonNullStmt asserts anyway, should I remove this?
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.
================
Comment at: clang/include/clang/AST/Stmt.h:1259
+ // the index of the last one.
+ unsigned getIndexOfLastNonNullStmt() const {
+ assert(!body_empty() && "getIndexOfLastNonNullStmt");
----------------
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`.
================
Comment at: clang/include/clang/AST/Stmt.h:1309
+ Stmt *getStmtExprResult() const {
+ unsigned ExprResult = getIndexOfLastNonNullStmt();
+ return body_begin()[ExprResult];
----------------
And if `getLastIndexOfNonNullStmt()` returns an empty value here, you can return `nullptr` to signify that there is no statement expression result possible.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:390
+ E = S.body_end(); I != E; ++I) {
+ if (GetLast && ExprResult == *I) {
+ // We have to special case labels here. They are statements, but when put
----------------
domdom wrote:
> aaron.ballman wrote:
> > What happens if `ExprResult` is `nullptr`?
> Then ExprResult == *I should not be evaluated. (Since GetLast would be false)
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).
================
Comment at: clang/test/CodeGen/exprs.c:202
+int f19() {
+ return ({ 3;;4;; });
+}
----------------
domdom wrote:
> aaron.ballman wrote:
> > Does this test need the extra null statement between the `3;` and `4;`?
> Not strictly speaking, no. Just added it to ensure it has no effect.
That sounds good to me!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57086/new/
https://reviews.llvm.org/D57086
More information about the cfe-commits
mailing list