ymandel marked 3 inline comments as done. ymandel added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:270 + // needed. + BoolValue &ComparisonValue = MakeValue(Env, *HasValueVal); + auto *ComparisonExprLoc = ---------------- xazax.hun wrote: > Is this the right way to initialize `ComparisonValue`? > > Considering the expression: `opt.value_or(nullptr) != nullptr` > * When `has_value == false`, `opt.value_or(nullptr)` will return `nullptr`, > so `!=` evaluates to false. This case seems to check out. > * However, when `has_value == true`, `opt` might still hold an `nullptr` and > `!=` could still evaluate to false. Thanks for digging into this. I think it's correct, but helpful to step through: Its correctness depends on `MakeValue`, so I'll focus on that in particular. For the `nullptr` case, we'll get: ``` HasValueVal && ContentsNotEqX ``` So, when `has_value == true`, this basically reduces to `ContentsNotEqX`. Since that's an atom, the result is indeterminate, which I believe is the desired outcome. WDYT? Also, even if I've convinced you, please let me know how i can improve the comments. For that matter, would `MakeValue` be better with a more specific name, like "MakePredicate" or somesuch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122231/new/ https://reviews.llvm.org/D122231 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
