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