[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 accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM with or without the `SpeculativeEvaluationRAII` refactor (which is ultimately pre-existing duplication).

Please commit the rename of `keepEvaluatingAfterFailure` -> `noteFailure` separately.


================
Comment at: lib/AST/ExprConstant.cpp:4104-4105
@@ -4072,4 +4103,4 @@
       return false;
     }
 
     Expr *EvalExpr = BoolResult ? E->getTrueExpr() : E->getFalseExpr();
----------------
The relevant case is when we're checking the body of a `constexpr` function to see whether it could possibly be a constant expression. In that case, what we do next should depend on whether we've already found something that is known to never be constant. If so (which is indicated by us having a diagnostic), we should bail out. If not, we should check both arms of the conditional operator and report a problem if neither of them can be constant.

I think the upshot of all of that is that `keepEvaluatingAfterFailure` should return false for `EM_PotentialConstantExpression*` if we have already produced a diagnostic. But that's not relevant for this particular patch, so let's leave it as you have it for now. We can revisit this.

================
Comment at: lib/AST/ExprConstant.cpp:7103
@@ -7067,2 +7102,3 @@
     Expr::EvalStatus OldEvalStatus;
+    bool WasSpecEval;
   };
----------------
george.burgess.iv wrote:
> 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.
Yes, if we can reuse `SpeculativeEvaluationRAII` here, or otherwise factor out the duplication, that would be better. Also I think you're probably right that this is not observable right now.

================
Comment at: test/SemaCXX/constant-expression-cxx1y.cpp:942
@@ +941,3 @@
+  // Ensure that we don't try to speculatively evaluate writes.
+  constexpr int add(int a, int b) { return a + b; }
+
----------------
Unused?


http://reviews.llvm.org/D18540