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