xazax.hun accepted this revision.
xazax.hun added inline comments.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:288
+bool isEngagedOptionalValue(const Value &OptionalVal, const Environment &env) {
+ auto *HasValueVal =
----------------
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.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:689
+ }
+ setHasValue(MergedVal, HasValueVal);
+ return true;
----------------
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.
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
