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