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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
