yronglin added a comment.
In D153296#4444612 <https://reviews.llvm.org/D153296#4444612>, @erichkeane
wrote:
> So I think I'm pretty confident that the only time we would call
> `EvaluateDependentExpr` is when we are in an error condition, so I'm
> convinced the fix 'as is' is incorrect. The check for noteSideEffect records
> that we HAVE a side effect, then returns if we are OK ignoring them right now.
>
> So since we are in a state where ignoring this error-case is acceptable, I
> think returning early there is incorrect as well, at least from a 'code
> correctness' (even if there isn't a reproducer that would matter?). I think
> we're in a case where we want to continue in order to ensure we go through
> the entire flow, so I THINK we should treat this as 'we have a value we don't
> know, so its just not found', and should fall into the check on 5019 (unless
> of course, there is a 'default' option!).
>
> So I think that we should be checking if `Value` is valid right after the
> default check, which lets us fall into the 'default' branch and get
> additional diagnostics/continued evaluation. WDYT @shafik / @yronglin ?
Erich, do you mean we do a modification like this？If I'm not misunderstand, I
think this looks good to me, we can get more diagnostics.
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f1f89122d4cc..967695c61df5 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -4984,8 +4984,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult
&Result, EvalInfo &Info,
return ESR_Failed;
if (SS->getCond()->isValueDependent()) {
// Stop evaluate if condition expression contains errors.
- if (SS->getCond()->containsErrors() ||
- !EvaluateDependentExpr(SS->getCond(), Info))
+ if (!EvaluateDependentExpr(SS->getCond(), Info))
return ESR_Failed;
} else {
if (!EvaluateInteger(SS->getCond(), Value, Info))
@@ -4995,6 +4994,8 @@ static EvalStmtResult EvaluateSwitch(StmtResult
&Result, EvalInfo &Info,
return ESR_Failed;
}
+ bool CondHasSideEffects = SS->getCond()->HasSideEffects(Info.getCtx());
+
// Find the switch case corresponding to the value of the condition.
// FIXME: Cache this lookup.
const SwitchCase *Found = nullptr;
@@ -5009,7 +5010,7 @@ static EvalStmtResult EvaluateSwitch(StmtResult
&Result, EvalInfo &Info,
APSInt LHS = CS->getLHS()->EvaluateKnownConstInt(Info.Ctx);
APSInt RHS = CS->getRHS() ? CS->getRHS()->EvaluateKnownConstInt(Info.Ctx)
: LHS;
- if (LHS <= Value && Value <= RHS) {
+ if (!CondHasSideEffects && LHS <= Value && Value <= RHS) {
Found = SC;
break;
}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153296/new/
https://reviews.llvm.org/D153296
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
