sgatev added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:270 + // needed. + BoolValue &ComparisonValue = MakeValue(Env, *HasValueVal); + auto *ComparisonExprLoc = ---------------- ymandel wrote: > ymandel wrote: > > xazax.hun wrote: > > > xazax.hun wrote: > > > > ymandel wrote: > > > > > 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? > > > > I think what confuses me is that we do something different for the 3 > > > > cases. You convinced me that `HasValueVal && ContentsNotEqX` is > > > > correct. But we only do this for one branch out of the 3. What is the > > > > reason for that? > > > Oh, never mind. Yeah, I think changing `MakeValue` to `MakePredicate` > > > would make this a bit clearer. After a second read now I understand > > > better what is going on. > > Just to be clear: the three cases you mean are lines 273-283, or something > > else? > and never mind my question, then (I rpelied before I saw your updated). I'll > change the name and add comments. Can you elaborate on the three cases on lines 273-283? Why not simply do ``` auto &ComparisonExprLoc = Env.createStorageLocation(*ComparisonExpr); Env.setStorageLocation(ComparisonExpr, ComparisonExprLoc); Env.setValue(ComparisonExprLoc, ComparisonValue); ``` 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
