[PATCH] D122231: [clang][dataflow] Add support for `value_or` in a comparison.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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:
> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122231/new/

https://reviews.llvm.org/D122231