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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296