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

[Yurong via Phabricator via cfe-commits]

yronglin marked 3 inline comments as done.
yronglin added a comment.

Thanks a lot for your comments! @hokein



================
Comment at: clang/lib/AST/ExprConstant.cpp:4921
+  // value is.
+  if (isa<RecoveryExpr>(E))
+    return false;
----------------
hokein wrote:
> The constant evaluator is not aware of the "error" concept, it is only aware of value-dependent -- the general idea behind is that we treat the dependent-on-error and dependent-on-template-parameters cases the same, they are potentially constant (if we see an expression contains errors, it could be constant depending on how the error is resolved), this will give us nice recovery and avoid bogus following diagnostics.
> 
> So, a `RecoveryExpr` should not result in failure when checking for a potential constant expression.
> 
> I think the right fix is to remove the conditional check `if (!EvaluateDependentExpr(SS->getCond(), Info))` in `EvaluateSwitch`, and return `ESR_Failed` unconditionally (we don't know its value, any switch-case anwser will be wrong in some cases). We already do this for return-statment, do-statement etc.
> 
> 
Do you mean?
```
if (SS->getCond()->isValueDependent()) {
    EvaluateDependentExpr(SS->getCond(), Info);
    return ESR_Failed;
}
```


================
Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:91
+
+static_assert(test13(), "should not crash"); // expected-error {{static assertion expression is not an integral constant expression}}
+
----------------
hokein wrote:
> nit: we can simplify it with the `TEST_EVALUATE` macro:
> 
> ```
> TEST_EVALUATE(SwitchErrorCond, switch(undef) { case 0: return 7; default: break;})
> ```
Thanks, I will use `TEST_EVALUATE ` to simplify.


================
Comment at: clang/test/SemaCXX/constexpr-function-recovery-crash.cpp:93
+
+constexpr int test14() {
+    int sum = 0;
----------------
hokein wrote:
> Is this a new crash (and the tests below)?
> 
> They don't look like new crashes, I think the current constant evaluator should be able to handle them well. IIUC the only crash we have is the case where we have a error-dependent condition in `switch`?
Thanks you for catch this, it's my mistake, I have ever copied these tests from the code above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153296