[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#4468373 <https://reviews.llvm.org/D153296#4468373>, @aaron.ballman wrote:

> In D153296#4459769 <https://reviews.llvm.org/D153296#4459769>, @yronglin wrote:
>
>> Please help me, I have no better idea on this issue, do you have any better ideas? @erichkeane @shafik
>
> I think what's being suggested is to change `EvaluateDependentExpr()` somewhat along these lines:
>
>   static bool EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
>     assert(E->isValueDependent());
>   
>     // Note that we have a side effect that matters for constant evaluation.
>     bool SideEffects = Info.noteSideEffect();
>     // If the reason we're here is because of a recovery expression, we don't
>     // want to continue to evaluate further as we will never know what the actual
>     // value is.
>     if (isa<RecoveryExpr>(E))
>       return false;
>   
>     // Otherwise, return whether we want to continue after noting the side
>     // effects, which should only happen if the expression has errors but isn't
>     // a recovery expression on its own.
>     assert(E->containsErrors() && "valid value-dependent expression should never "
>                                   "reach invalid code path.");
>     return SideEffects;
>   }
>
> This way, code paths that get down to a `RecoveryExpr` will not continue to evaluate further (as there's really no point -- there's no way to get a reasonable value from from the recovery expression anyway), but the fix isn't specific to just switch statements. After making these changes, you should look for places where `EvaluateDependentExpr()` is being called to try to come up with a test case where that expression is a recovery expression so that we can fill out test coverage beyond just the situation with `switch` from the original report. Does that make sense?
>
> (Marking as requesting changes so it's clear this review isn't yet accepted.)

Thanks a lot! @aaron.ballman , I try to address comments and add more test, this case (https://godbolt.org/z/ExPoEKcrf) looks strange, why the do-statement missing in the printed AST?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153296/new/

https://reviews.llvm.org/D153296