sgatev added inline comments.
================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:170 /// Returns the symbolic value that represents the "has_value" property of the -/// optional value `Val`. Returns null if `Val` is null. -BoolValue *getHasValue(Value *Val) { +/// optional value `Val`, creating a fresh one if none is set. Returns null if +/// `Val` is null. ---------------- Why is this necessary? Do we really need to create a new value on each `getHasValue` call? Can we centralize initialization to avoid this? I'm worried that "has_value" and "value" initialization isn't coordinated. ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:219 +/// its location. Returns nullptr if the value can't be represented. +StorageLocation *initializeUnwrappedValue(const Expr &Expr, + StructValue &OptionalVal, ---------------- Perhaps call this `maybeInitializeOptionalValueMember`? ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:219 +/// its location. Returns nullptr if the value can't be represented. +StorageLocation *initializeUnwrappedValue(const Expr &Expr, + StructValue &OptionalVal, ---------------- sgatev wrote: > Perhaps call this `maybeInitializeOptionalValueMember`? Let's pass the type instead of `Expr`, if that's the only thing we need. ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:222-224 + // The "value" property represents a synthetic field. As such, it needs + // `StorageLocation`, like normal fields (and other variables). So, we model + // it with a `ReferenceValue`, since that includes a storage location. Once ---------------- It's unclear to me why we "need" a `StorageLocation`. Representing the value as a `ReferenceValue` seems good to me, but there are other options. What issues are we running into if we remove the indirection? ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:231-236 + // The property was previously set, but the value has been lost. This can + // happen, for example, because of an environment merge (where the two + // environments mapped the property to different values, which resulted in + // them both being discarded), or when two blocks in the CFG, with neither + // a dominator of the other, visit the same optional value, or even when a + // block is revisited during testing to collect per-statement state. ---------------- So we don't lose the `ReferenceValue`, but we do lose its pointee? Is that the behavior we want when merging? ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:281 + + auto *Prop = OptionalVal->getProperty("has_value"); + if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) { ---------------- Shouldn't we use `getHasValue` here? ================ Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2188-2260 + // `reset` changes the state. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional<std::string> opt; ---------------- Do we really need tests for all members that check/access/reset the content? These are already covered in tests above. ================ Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2280 + void target() { + Bar bar = createBar(); + bar.member->opt = "a"; ---------------- ================ Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2287 + + // Resetting the nested optional. + ExpectLatticeChecksFor( ---------------- Do we really need this one or is it already covered by one of the tests in this file? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124932/new/ https://reviews.llvm.org/D124932 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits