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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
