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

[George Burgess IV via cfe-commits]

george.burgess.iv added inline comments.

================
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.
     }
----------------
rsmith wrote:
> I think this comment is now redundant / incorrect.
My bad :)

================
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;
----------------
rsmith wrote:
> 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?
The best alternative I can come up with is `noteFailureAndGetShouldContinueEvaluating()`, which occupies 44 characters.

...I think I'll go back to the old name.

================
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);
----------------
rsmith wrote:
> 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).
I'm not sure that I fully understand what you mean by this. Are you saying that we (ultimately) only want to check the arms of a conditional when we're evaluating for overflow?

Either way, took a stab at fixing it.

================
Comment at: lib/AST/ExprConstant.cpp:7103
@@ -7067,2 +7102,3 @@
     Expr::EvalStatus OldEvalStatus;
+    bool WasSpecEval;
   };
----------------
I'm happy to make SpeculativeEvalRAII a type that optionally restores state, if you think that would be better. That way, we don't need to duplicate this logic...

Also, I can't figure out how to test this, because it's only ever run after `HasSideEffects = true`, and the only thing that (I think) checks for if we're speculatively evaluating *also* fails in exactly the same way if we have side effects.


http://reviews.llvm.org/D18540