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