[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Considering that this has been fertile ground for buggy edge cases, should we revert my original changes from the 8.0 branch to give this more time to bake on trunk before releasing? Or do you think this is fine to move onto the release branch once finalized?



================
Comment at: include/clang/AST/Stmt.h:1598-1602
+  const Expr *getExprStmt() const;
+  Expr *getExprStmt() {
+    const ValueStmt *ConstThis = this;
+    return const_cast<Expr*>(ConstThis->getExprStmt());
+  }
----------------
rsmith wrote:
> aaron.ballman wrote:
> > We typically implement this in reverse, where the non-const version holds the actual implementation and the const version performs a `const_cast`.
> We do. Do you think that's preferable? I like that this way around proves that the `const` version is const-correct, but it's a tiny benefit and I'm fine with just being consistent.
Personally, I prefer the way you have it here (though I wish we had a global helper function to hide the dispatch dance).


================
Comment at: include/clang/Parse/Parser.h:374
+  /// a statement expression and builds a suitable expression statement.
+  StmtResult handleExprStmt(ExprResult E, WithinStmtExpr IsInStmtExpr);
 
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Rather than passing around `IsInStmtExpr` to a bunch of parser APIs, would it make more sense to add an RAII object that sets some state on `Parser` to test it? The proliferation of arguments seems unfortunate given how much of a corner case statement expressions are.
> Yeah, I tried that approach first, but the parser state turns out to be much worse, because it puts a burden on every form of statement that constructs a nested parsing context to reset that state. We can put the resetting on `ParseScope`, but it still needs to have an effect in the case where the scope is disabled, which is surprising, and there's no guarantee such statement constructs that can end in an expression will have a nested scope (consider, for instance, constructs like `return x;` or `goto *label;`). This is really local state that should only be propagated through a very small number of syntactic constructs rather than global state.
> 
> Maybe we could combine the new flag with the `AllowOpenMPStandalone` / `AllowedConstructsKind` flag into a more general kind of "statement context". The propagation rules aren't quite the same (`AllowOpenMPStandalone` doesn't get propagated through labels whereas `IsInStmtExpr` does), which is a little awkward, but maybe that's not so bad -- and maybe that's actually a bug in the OpenMP implementation?
> Maybe we could combine the new flag with the AllowOpenMPStandalone / AllowedConstructsKind flag into a more general kind of "statement context".

That seems like a sensible approach to me.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57984/new/

https://reviews.llvm.org/D57984