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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

@ABataev Is it intentional that we do not propagate `Allowed` through labels? For example:

  void f() {
    #pragma omp barrier // ok
  
  label:
    #pragma omp barrier // error, "cannot be an immediate substatement"
  
  label:
    ;
    #pragma omp barrier // ok
  }

?



================
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());
+  }
----------------
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.


================
Comment at: include/clang/Parse/Parser.h:374
+  /// a statement expression and builds a suitable expression statement.
+  StmtResult handleExprStmt(ExprResult E, WithinStmtExpr IsInStmtExpr);
 
----------------
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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57984