[clang][dataflow] Eliminate RecordValue::getChild(). (#65586)

We want to eliminate the `RecordStorageLocation` from `RecordValue` and,
ultimately, eliminate `RecordValue` entirely (see the discussion linked
in the
`RecordValue` class comment). This is one step in that direction.

To eliminate `RecordValue::getChild()`, we also eliminate the last
remaining
caller, namely the `getFieldValue(const RecordValue *, ...)` overload.
Calls
to this overload have been rewritten to use the
`getFieldValue(const RecordStorageLocation *, ...)` overload. Note that
this
also makes the code slightly simpler in many cases.

Author: martinboehme

clang/include/clang/Analysis/FlowSensitive/Value.h

@@ -225,12 +225,6 @@ class RecordValue final : public Value {
   /// Returns the storage location that this `RecordValue` is associated with.
   RecordStorageLocation &getLoc() const { return Loc; }
 
-  /// Convenience function that returns the child storage location for `Field`.
-  /// See also the documentation for `RecordStorageLocation::getChild()`.
-  StorageLocation *getChild(const ValueDecl &Field) const {
-    return Loc.getChild(Field);
-  }
-
 private:
   RecordStorageLocation &Loc;
 };

clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp

@@ -91,8 +91,8 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) {
   // Verify that the struct and the field (`R`) with first appearance of the
   // type is created successfully.
   Environment Env(DAContext, *Fun);
-  RecordValue *SVal = cast<RecordValue>(Env.createValue(Ty));
-  PointerValue *PV = cast_or_null<PointerValue>(getFieldValue(SVal, *R, Env));
+  auto &SLoc = cast<RecordStorageLocation>(Env.createObject(Ty));
+  PointerValue *PV = cast_or_null<PointerValue>(getFieldValue(&SLoc, *R, Env));
   EXPECT_THAT(PV, NotNull());
 }
 
@@ -240,8 +240,8 @@ TEST_F(EnvironmentTest, IncludeFieldsFromDefaultInitializers) {
   // Verify that the `X` field of `S` is populated when analyzing the
   // constructor, even though it is not referenced directly in the constructor.
   Environment Env(DAContext, *Constructor);
-  auto *Val = cast<RecordValue>(Env.createValue(QTy));
-  EXPECT_THAT(getFieldValue(Val, *XDecl, Env), NotNull());
+  auto &Loc = cast<RecordStorageLocation>(Env.createObject(QTy));
+  EXPECT_THAT(getFieldValue(&Loc, *XDecl, Env), NotNull());
 }
 
 TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) {
@@ -285,8 +285,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) {
   Environment Env(DAContext, *Fun);
   const auto *GlobalLoc =
       cast<RecordStorageLocation>(Env.getStorageLocation(*GlobalDecl));
-  const auto *GlobalVal = cast<RecordValue>(Env.getValue(*GlobalLoc));
-  auto *BarVal = getFieldValue(GlobalVal, *BarDecl, Env);
+  auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env);
   EXPECT_TRUE(isa<IntegerValue>(BarVal));
 }
 

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

@@ -463,18 +463,6 @@ inline Value *getFieldValue(const RecordStorageLocation *Loc,
   return Env.getValue(*FieldLoc);
 }
 
-/// Returns the value of a `Field` on a `Struct.
-/// Returns null if `Struct` is null.
-inline Value *getFieldValue(const RecordValue *Struct, const ValueDecl &Field,
-                            const Environment &Env) {
-  if (Struct == nullptr)
-    return nullptr;
-  StorageLocation *FieldLoc = Struct->getChild(Field);
-  if (FieldLoc == nullptr)
-    return nullptr;
-  return Env.getValue(*FieldLoc);
-}
-
 /// Creates and owns constraints which are boolean values.
 class ConstraintContext {
   unsigned NextAtom = 0;

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

@@ -647,26 +647,26 @@ TEST(TransferTest, SelfReferentialPointerVarDecl) {
         const auto &FooLoc =
             *cast<ScalarStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto &FooVal = *cast<PointerValue>(Env.getValue(FooLoc));
-        const auto &FooPointeeVal =
-            *cast<RecordValue>(Env.getValue(FooVal.getPointeeLoc()));
+        const auto &FooPointeeLoc =
+            cast<RecordStorageLocation>(FooVal.getPointeeLoc());
 
         const auto &BarVal =
-            *cast<PointerValue>(getFieldValue(&FooPointeeVal, *BarDecl, Env));
-        const auto &BarPointeeVal =
-            *cast<RecordValue>(Env.getValue(BarVal.getPointeeLoc()));
+            *cast<PointerValue>(getFieldValue(&FooPointeeLoc, *BarDecl, Env));
+        const auto &BarPointeeLoc =
+            cast<RecordStorageLocation>(BarVal.getPointeeLoc());
 
-        EXPECT_THAT(getFieldValue(&BarPointeeVal, *FooRefDecl, Env), NotNull());
+        EXPECT_THAT(getFieldValue(&BarPointeeLoc, *FooRefDecl, Env), NotNull());
 
         const auto &FooPtrVal = *cast<PointerValue>(
-            getFieldValue(&BarPointeeVal, *FooPtrDecl, Env));
+            getFieldValue(&BarPointeeLoc, *FooPtrDecl, Env));
         const auto &FooPtrPointeeLoc =
             cast<RecordStorageLocation>(FooPtrVal.getPointeeLoc());
         EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull());
 
-        EXPECT_THAT(getFieldValue(&BarPointeeVal, *BazRefDecl, Env), NotNull());
+        EXPECT_THAT(getFieldValue(&BarPointeeLoc, *BazRefDecl, Env), NotNull());
 
         const auto &BazPtrVal = *cast<PointerValue>(
-            getFieldValue(&BarPointeeVal, *BazPtrDecl, Env));
+            getFieldValue(&BarPointeeLoc, *BazPtrDecl, Env));
         const StorageLocation &BazPtrPointeeLoc = BazPtrVal.getPointeeLoc();
         EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull());
       });
@@ -2478,9 +2478,10 @@ TEST(TransferTest, BindTemporary) {
         const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
         ASSERT_THAT(BazDecl, NotNull());
 
-        const auto &FooVal = *cast<RecordValue>(Env.getValue(*FooDecl));
+        const auto &FooLoc =
+            *cast<RecordStorageLocation>(Env.getStorageLocation(*FooDecl));
         const auto *BarVal = cast<IntegerValue>(Env.getValue(*BarDecl));
-        EXPECT_EQ(BarVal, getFieldValue(&FooVal, *BazDecl, Env));
+        EXPECT_EQ(BarVal, getFieldValue(&FooLoc, *BazDecl, Env));
       });
 }
 
@@ -2970,16 +2971,14 @@ TEST(TransferTest, AggregateInitialization) {
           const auto *BarArgVal = cast<IntegerValue>(Env.getValue(*BarArgDecl));
           const auto *QuxArgVal = cast<IntegerValue>(Env.getValue(*QuxArgDecl));
 
-          const auto *QuuxVal = cast<RecordValue>(Env.getValue(*QuuxDecl));
-          ASSERT_THAT(QuuxVal, NotNull());
-
-          const auto *BazVal =
-              cast<RecordValue>(getFieldValue(QuuxVal, *BazDecl, Env));
-          ASSERT_THAT(BazVal, NotNull());
+          const auto &QuuxLoc =
+              *cast<RecordStorageLocation>(Env.getStorageLocation(*QuuxDecl));
+          const auto &BazLoc =
+              *cast<RecordStorageLocation>(QuuxLoc.getChild(*BazDecl));
 
-          EXPECT_EQ(getFieldValue(QuuxVal, *BarDecl, Env), BarArgVal);
-          EXPECT_EQ(getFieldValue(BazVal, *FooDecl, Env), FooArgVal);
-          EXPECT_EQ(getFieldValue(QuuxVal, *QuxDecl, Env), QuxArgVal);
+          EXPECT_EQ(getFieldValue(&QuuxLoc, *BarDecl, Env), BarArgVal);
+          EXPECT_EQ(getFieldValue(&BazLoc, *FooDecl, Env), FooArgVal);
+          EXPECT_EQ(getFieldValue(&QuuxLoc, *QuxDecl, Env), QuxArgVal);
 
           // Check that fields initialized in an initializer list are always
           // modeled in other instances of the same type.
@@ -3653,8 +3652,9 @@ TEST(TransferTest, AssignMemberBeforeCopy) {
 
         const auto *BarVal = cast<IntegerValue>(Env.getValue(*BarDecl));
 
-        const auto *A2Val = cast<RecordValue>(Env.getValue(*A2Decl));
-        EXPECT_EQ(getFieldValue(A2Val, *FooDecl, Env), BarVal);
+        const auto &A2Loc =
+            *cast<RecordStorageLocation>(Env.getStorageLocation(*A2Decl));
+        EXPECT_EQ(getFieldValue(&A2Loc, *FooDecl, Env), BarVal);
       });
 }