[PATCH] D125931: [clang][dataflow] Add support for correlated branches to optional model

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:288
 
+bool isEngagedOptionalValue(const Value &OptionalVal, const Environment &env) {
+  auto *HasValueVal =
----------------
xazax.hun wrote:
> I wonder whether `NonEmpty` is clearer than `Engaged`. I think `Engaged` can also be OK, but we probably want to have a comment somewhere to explain the terminology.
Sounds good. I went with `isEmptyOptional` and `isNonEmptyOptional`.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:689
+  }
+  setHasValue(MergedVal, HasValueVal);
+  return true;
----------------
xazax.hun wrote:
> Would it make sense to have the converese? I.e., when both optionals are empty, create an empty optional here. While currently this modeling does not distinguish between unchecked and empty optionals it might be handy in the future. Although I am also happy with this as is, not adding any code that does not have any utility as of today.
Good idea! I think this can be useful. I added the code and a test case based on dead code:

```
if (b) {
  opt = std::nullopt;
} else {
  opt = std::nullopt;
}
if (opt.has_value()) {
  // unreachable
}
```


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:174
+/// property of the optional value `OptionalVal`.
+void setHasValue(StructValue &OptionalVal, BoolValue &HasValueVal) {
+  OptionalVal.setProperty("has_value", HasValueVal);
----------------
ymandel wrote:
> sgatev wrote:
> > ymandel wrote:
> > > I believe you can relax this to `Value` because `setProperty` is no longer specific to `StructValue`.
> > I did that intentionally because I still think it makes sense to assert that an optional is modeled as a `StructValue`. I haven't thought about where and how it'd be best to assert that though so I'll happily remove the casts for now.
> Sounds good. I had the same thought. But, it occurs to me that at this point, are we using the `StructValue` at all? If not, then the only reason we expect it to be a `StructValue` is because we know the optional type is a record type. But, that's not an assumption of our model. So, maybe we should be agnostic to the underlying representation?
Agreed. Let's not assume a `StructValue` representation unless it's necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125931