[PATCH] D18540: [Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/AST/ExprConstant.cpp:782
@@ +781,3 @@
+    LLVM_ATTRIBUTE_UNUSED_RESULT bool noteUnrecoverableFailure() {
+      // This is a little bit icky. We leave HasSideEffects unchange if we
+      // aren't going to keep running, because eval modes that don't allow
----------------
unchange -> unchanged

================
Comment at: lib/AST/ExprConstant.cpp:784-785
@@ +783,4 @@
+      // aren't going to keep running, because eval modes that don't allow
+      // evaluation after side-effects/failure sometimes depend on the value of
+      // HasSideEffects. This is why this function should only be called if we
+      // intend to keep evaluating.
----------------
I think this is the right behavior regardless of the current expectations of any particular caller. After this patch:

  * `HasSideEffects == false` indicates that the evaluation (if evaluation was successful, then up to the current point, otherwise up to the point where evaluation failed) has definitely not skipped any side-effects
  * `HasSideEffects == true` means nothing is known about side-effects (in particular, it does not guarantee that a side-effect has definitely happened, just that one /might/ have happened)

(If any caller expects these flags to mean something else, they are wrong -- and note that `HasSideEffects` is meaningless if evaluation fails.) Therefore, if we transition from an "evaluation has failed" to an "evaluation was successful" state, we must set `HasSideEffects` to `true` to model the subexpressions we might have skipped. And if we just unwind (because `keepEvaluatingAfterFailure()` returns `false`), then we don't need to set `HasSideEffects` at all.

Perhaps you could say something like this:

"Failure when evaluating some expression often means there is some subexpression whose evaluation was skipped. Therefore, (because we don't track whether we skipped an expression when unwinding after an evaluation failure) every evaluation failure that bubbles up from a subexpression implies that a side-effect has potentially happened. We skip setting the HasSideEffects flag to true until we decide to continue evaluating after that point, which happens here."

================
Comment at: lib/AST/ExprConstant.cpp:853-854
@@ -825,5 +852,4 @@
       Info.EvalStatus.Diag = NewDiag;
       // If we're speculatively evaluating, we may have skipped over some
       // evaluations and missed out a side effect.
     }
----------------
I think this comment is now redundant / incorrect.

================
Comment at: lib/AST/ExprConstant.cpp:2643
@@ +2642,3 @@
+  // Don't allow the speculative evaluation of writes.
+  if (AK != AK_Read && Info.IsSpeculativelyEvaluating)
+    return CompleteObject();
----------------
`IsSpeculativelyEvaluating` is always `false`. Presumably it should be set to `true` in the `SpeculativeEvaluationRAII` constructor. (Please also add a testcase to catch this...)

================
Comment at: lib/AST/ExprConstant.cpp:3280
@@ -3249,3 +3279,3 @@
   if (!EvaluateObjectArgument(Info, BO->getLHS(), LV)) {
-    if (Info.keepEvaluatingAfterFailure()) {
+    if (Info.noteUnrecoverableFailure()) {
       MemberPtr MemPtr;
----------------
Can we give this function a name that reads more naturally at the call site? It's locally unclear what this condition is testing. Maybe keep the old name?

================
Comment at: lib/AST/ExprConstant.cpp:4074-4076
@@ -4043,2 +4073,5 @@
     assert(Info.checkingPotentialConstantExpression());
+    assert(
+        Info.EvalStatus.HasSideEffects &&
+        "Need multiple speculative eval RAIIs below if we haven't failed yet.");
 
----------------
These RAII objects are cheap; maybe just create two of them regardless?

================
Comment at: lib/AST/ExprConstant.cpp:4104-4105
@@ -4069,1 +4103,4 @@
+        (void)KeepGoing;
+        assert(KeepGoing && "We can't evaluate after a failure in potential "
+                            "constant expressions?");
         CheckPotentialConstantConditional(E);
----------------
I think this should be an `if` rather than an `assert`. If we hit something that's known to be non-constant while evaluating the condition of a `?:`, we should bail out of evaluation in `checkingPotentialConstantExpression` mode. `keepEvaluatingAfterFailure` should return `false` in that case (even though it doesn't do so today).

================
Comment at: lib/AST/ExprConstant.cpp:7374-7375
@@ -7335,4 +7373,4 @@
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
-  if (!Info.keepEvaluatingAfterFailure() && E->isAssignmentOp())
+  if (E->isAssignmentOp() && !Info.noteUnrecoverableFailure())
     return Error(E);
 
----------------
The side-effect here should be delayed until after we've evaluated the left and right hand sides of the assignment.


http://reviews.llvm.org/D18540