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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Sema/SemaExprCXX.cpp:5636-5639
@@ -5634,4 +5635,6 @@
 
-  Expr *E = ExprWithCleanups::Create(Context, SubExpr, Cleanups);
+  auto *E = ExprWithCleanups::Create(Context, SubExpr, Cleanups);
+  if (Cleanup.cleanupsHaveSideEffects())
+    E->setCleanupsHaveSideEffects();
   DiscardCleanupsInEvaluationContext();
 
----------------
Please pass these flags into `::Create` rather than using a setter. Generally, we aim for the AST to be immutable once created.

================
Comment at: lib/Sema/SemaInit.cpp:6191
@@ +6190,3 @@
+  if (getLangOpts().CPlusPlus)
+    Cleanup.setExprNeedsCleanups(MTE->getType().isDestructedType());
+  return MTE;
----------------
I think this may set ExprNeedsCleanups even when unnecessary, if the MTE is lifetime-extended (I think you only actually want an ExprWithCleanups if the storage duration is SD_Automatic). We can't really deal with that here, because we don't know the storage duration of the MTE yet, but please add a FIXME here to not set ExprNeedsCleanups if the temporary is lifetime-extended.


http://reviews.llvm.org/D20498