[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

[Yurong via Phabricator via cfe-commits]

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