shafik 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 ?
I do see what you are saying but I think that we have similar cases where
without a value the next step is impossible to do for example in `EvaluateStmt`
the `case Stmt::ReturnStmtClass:` case:
case Stmt::ReturnStmtClass: {
const Expr *RetExpr = cast<ReturnStmt>(S)->getRetValue();
FullExpressionRAII Scope(Info);
if (RetExpr && RetExpr->isValueDependent()) {
EvaluateDependentExpr(RetExpr, Info);
// We know we returned, but we don't know what the value is.
return ESR_Failed;
}
We can't return a value we can't calculate right? and here as well for the
`Stmt::DoStmtClass` case
if (DS->getCond()->isValueDependent()) {
EvaluateDependentExpr(DS->getCond(), Info);
// Bailout as we don't know whether to keep going or terminate the loop.
return ESR_Failed;
}
This case feels the same as the two above, we can't calculate the jump for the
switch if we can't calculate the value.
CC @rsmith
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153296/new/
https://reviews.llvm.org/D153296
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
