[PATCH] D122273: [clang][dataflow] Fix handling of base-class fields

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:59-60
 /// can be traced independently by abstract interpretation. For example: a
-/// struct with public members.
+/// struct with public members. Note that the corresponding `StructValue` has a
+/// flat layout that does not contain the child locations stored here.
 class AggregateStorageLocation final : public StorageLocation {
----------------
I find this a bit confusing. `StructValue` does not contain storage locations in general. I think we should make it clear that the layout of `AggregateStorageLocation` is flat, i.e. if it's used for a `struct` or `class` type it will contain child storage locations for all accessible members of base `struct` and `class` types.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:193-194
+/// Models a value of `struct` or `class` type. Implements a flat struct layout,
+/// where the child values are directly reachable from the struct value (rather
+/// than indirectly, through `StorageLocation`s).
 class StructValue final : public Value {
----------------
I'm not sure what's meant here by indirect reachability, but I suggest documenting that a `StructValue` that models a `struct` or `class` type contains child values for all accessible members of its base `struct` and `class` types. 


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1097-1100
+        const auto *FooLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*FooDecl, SkipPast::None));
+        const auto &FooVal = *cast<StructValue>(Env.getValue(*FooLoc));
+        EXPECT_THAT(FooVal.getChild(*APublicDecl), NotNull());
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1102-1111
+        // Base-class fields.
+        EXPECT_THAT(FooVal.getChild(*ADefaultDecl), IsNull());
+        EXPECT_THAT(FooVal.getChild(*APrivateDecl), IsNull());
+        EXPECT_THAT(FooVal.getChild(*AProtectedDecl), NotNull());
+        EXPECT_THAT(FooVal.getChild(*APublicDecl), NotNull());
+
+        // Derived-class fields.
----------------
Let's also check `FooLoc`'s child storage locations. Same for the test below.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1156-1159
+        const auto *FooLoc = cast<AggregateStorageLocation>(
+            Env.getStorageLocation(*FooDecl, SkipPast::None));
+        const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc));
+        EXPECT_THAT(FooVal->getChild(*BarDecl), NotNull());
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122273