[PATCH] D20498: [Temporary] Add an ExprWithCleanups for each C++ MaterializeTemporaryExpr

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: include/clang/AST/ExprCXX.h:2871-2872
@@ -2870,1 +2870,4 @@
 
+  // When false, it must not have side effects.
+  bool CleanupsHaveSideEffects;
+
----------------
You may as well put this in the bitfield storage on `Stmt`.

================
Comment at: include/clang/Sema/CleanupInfo.h:31
@@ +30,3 @@
+    ExprNeedsCleanups = true;
+    CleanupsHaveSideEffects = SideEffects;
+  }
----------------
Should this be `|=`?

================
Comment at: include/clang/Sema/Sema.h:444
@@ -442,4 +443,3 @@
 
-  /// ExprNeedsCleanups - True if the current evaluation context
-  /// requires cleanups to be run at its conclusion.
-  bool ExprNeedsCleanups;
+  /// Used to control the generation of ExprNeedsCleanups.
+  CleanupInfo Cleanup;
----------------
ExprNeedsCleanups -> ExprWithCleanups

================
Comment at: lib/AST/Expr.cpp:2906-2907
@@ +2905,4 @@
+  case ExprWithCleanupsClass:
+    if (cast<ExprWithCleanups>(this)->cleanupsHaveSideEffects())
+      return true;
+    break;
----------------
You should only return `true` if `IncludePossibleEffects` -- `cleanupsHaveSideEffects` means there might be a side-effect, not that there definitely is one. (This is a pre-existing bug.)

================
Comment at: lib/Sema/SemaExpr.cpp:4579
@@ -4578,3 +4578,3 @@
     // any explicit objects.
-    ExprNeedsCleanups = true;
+    Cleanup.setExprNeedsCleanups(true);
 
----------------
This should inherit the side-effects flag from the `ExprWithCleanups` in the default argument.

================
Comment at: lib/Sema/SemaInit.cpp:6190-6191
@@ +6189,4 @@
+      MaterializeTemporaryExpr(T, Temporary, BoundToLvalueReference);
+  if (getLangOpts().CPlusPlus)
+    Cleanup.setExprNeedsCleanups(MTE->getType().isDestructedType());
+  return MTE;
----------------
Please also sink the calls to mark the destructor referenced and check its access into here too. We should keep the checks for a usable destructor and the triggering of an ExprWithCleanups for the destructor together.


http://reviews.llvm.org/D20498