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

Thank you for working on this! It looks like we did have to add the new AST node after all, but the changes seem pretty good. Should anything be updated in the AST dumper for this (such as printing whether a Stmt is a value-producing statement or not)?



================
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());
+  }
----------------
We typically implement this in reverse, where the non-const version holds the actual implementation and the const version performs a `const_cast`.


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


================
Comment at: lib/Sema/SemaExpr.cpp:13319
   if (!Compound->body_empty()) {
-    Stmt *LastStmt = Compound->body_back();
-    LabelStmt *LastLabelStmt = nullptr;
-    // If LastStmt is a label, skip down through into the body.
-    while (LabelStmt *Label = dyn_cast<LabelStmt>(LastStmt)) {
-      LastLabelStmt = Label;
-      LastStmt = Label->getSubStmt();
-    }
-
-    if (Expr *LastE = dyn_cast<Expr>(LastStmt)) {
-      // Do function/array conversion on the last expression, but not
-      // lvalue-to-rvalue.  However, initialize an unqualified type.
-      ExprResult LastExpr = DefaultFunctionArrayConversion(LastE);
-      if (LastExpr.isInvalid())
-        return ExprError();
-      Ty = LastExpr.get()->getType().getUnqualifiedType();
-
-      if (!Ty->isDependentType() && !LastExpr.get()->isTypeDependent()) {
-        // In ARC, if the final expression ends in a consume, splice
-        // the consume out and bind it later.  In the alternate case
-        // (when dealing with a retainable type), the result
-        // initialization will create a produce.  In both cases the
-        // result will be +1, and we'll need to balance that out with
-        // a bind.
-        if (Expr *rebuiltLastStmt
-              = maybeRebuildARCConsumingStmt(LastExpr.get())) {
-          LastExpr = rebuiltLastStmt;
-        } else {
-          LastExpr = PerformCopyInitialization(
-              InitializedEntity::InitializeStmtExprResult(LPLoc, Ty),
-              SourceLocation(), LastExpr);
-        }
-
-        if (LastExpr.isInvalid())
-          return ExprError();
-        if (LastExpr.get() != nullptr) {
-          if (!LastLabelStmt)
-            Compound->setLastStmt(LastExpr.get());
-          else
-            LastLabelStmt->setSubStmt(LastExpr.get());
-          StmtExprMayBindToTemp = true;
-        }
+    if (ValueStmt *LastStmt = dyn_cast<ValueStmt>(Compound->body_back())) {
+      if (Expr *Value = LastStmt->getExprStmt()) {
----------------
`const auto *`


================
Comment at: lib/Sema/SemaExpr.cpp:13355
+  // a bind.
+  ImplicitCastExpr *Cast = dyn_cast<ImplicitCastExpr>(E);
+  if (Cast && Cast->getCastKind() == CK_ARCConsumeObject)
----------------
`auto *`


Repository:
  rC Clang

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

https://reviews.llvm.org/D57984