[clang][dataflow] Eliminate RecordValue::getChild().
#65586
Merged
+27
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
clang:dataflow
kinu
reviewed
Sep 7, 2023
ymand
approved these changes
Sep 7, 2023
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.
martinboehme
force-pushed
the
piper_export_cl_563364678
branch
from
cd8f6b6 to
2b2bedd
Compare
|
Rebasing so I can run the CI again at head (and it will hopefully pass). |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis ChangesWe want to eliminate the To eliminate -- 4 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h
index 6e911af7264ced9..e6c68e5b4e93e1e 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Value.h
+++ b/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;
};
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 6e37011a052c5e4..ad40c0b082d302e 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/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(Env.createValue(Ty));
- PointerValue *PV = cast_or_null(getFieldValue(SVal, *R, Env));
+ auto &SLoc = cast(Env.createObject(Ty));
+ PointerValue *PV = cast_or_null(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(Env.createValue(QTy));
- EXPECT_THAT(getFieldValue(Val, *XDecl, Env), NotNull());
+ auto &Loc = cast(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(Env.getStorageLocation(*GlobalDecl));
- const auto *GlobalVal = cast(Env.getValue(*GlobalLoc));
- auto *BarVal = getFieldValue(GlobalVal, *BarDecl, Env);
+ auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env);
EXPECT_TRUE(isa(BarVal));
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
index c61e9f26beff40b..0cf66c218f43fdc 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ b/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;
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index cced3925c4721c5..05ee5df0e95a433 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -647,26 +647,26 @@ TEST(TransferTest, SelfReferentialPointerVarDecl) {
const auto &FooLoc =
*cast(Env.getStorageLocation(*FooDecl));
const auto &FooVal = *cast(Env.getValue(FooLoc));
- const auto &FooPointeeVal =
- *cast(Env.getValue(FooVal.getPointeeLoc()));
+ const auto &FooPointeeLoc =
+ cast(FooVal.getPointeeLoc());
const auto &BarVal =
- *cast(getFieldValue(&FooPointeeVal, *BarDecl, Env));
- const auto &BarPointeeVal =
- *cast(Env.getValue(BarVal.getPointeeLoc()));
+ *cast(getFieldValue(&FooPointeeLoc, *BarDecl, Env));
+ const auto &BarPointeeLoc =
+ cast(BarVal.getPointeeLoc());
- EXPECT_THAT(getFieldValue(&BarPointeeVal, *FooRefDecl, Env), NotNull());
+ EXPECT_THAT(getFieldValue(&BarPointeeLoc, *FooRefDecl, Env), NotNull());
const auto &FooPtrVal = *cast(
- getFieldValue(&BarPointeeVal, *FooPtrDecl, Env));
+ getFieldValue(&BarPointeeLoc, *FooPtrDecl, Env));
const auto &FooPtrPointeeLoc =
cast(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(
- 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(Env.getValue(*FooDecl));
+ const auto &FooLoc =
+ *cast(Env.getStorageLocation(*FooDecl));
const auto *BarVal = cast(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(Env.getValue(*BarArgDecl));
const auto *QuxArgVal = cast(Env.getValue(*QuxArgDecl));
- const auto *QuuxVal = cast(Env.getValue(*QuuxDecl));
- ASSERT_THAT(QuuxVal, NotNull());
-
- const auto *BazVal =
- cast(getFieldValue(QuuxVal, *BazDecl, Env));
- ASSERT_THAT(BazVal, NotNull());
+ const auto &QuuxLoc =
+ *cast(Env.getStorageLocation(*QuuxDecl));
+ const auto &BazLoc =
+ *cast(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(Env.getValue(*BarDecl));
- const auto *A2Val = cast(Env.getValue(*A2Decl));
- EXPECT_EQ(getFieldValue(A2Val, *FooDecl, Env), BarVal);
+ const auto &A2Loc =
+ *cast(Env.getStorageLocation(*A2Decl));
+ EXPECT_EQ(getFieldValue(&A2Loc, *FooDecl, Env), BarVal);
});
}
|
|
@llvm/pr-subscribers-clang ChangesWe want to eliminate the To eliminate -- 4 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h
index 6e911af7264ced9..e6c68e5b4e93e1e 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Value.h
+++ b/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;
};
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 6e37011a052c5e4..ad40c0b082d302e 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/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(Env.createValue(Ty));
- PointerValue *PV = cast_or_null(getFieldValue(SVal, *R, Env));
+ auto &SLoc = cast(Env.createObject(Ty));
+ PointerValue *PV = cast_or_null(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(Env.createValue(QTy));
- EXPECT_THAT(getFieldValue(Val, *XDecl, Env), NotNull());
+ auto &Loc = cast(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(Env.getStorageLocation(*GlobalDecl));
- const auto *GlobalVal = cast(Env.getValue(*GlobalLoc));
- auto *BarVal = getFieldValue(GlobalVal, *BarDecl, Env);
+ auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env);
EXPECT_TRUE(isa(BarVal));
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
index c61e9f26beff40b..0cf66c218f43fdc 100644
--- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ b/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;
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index cced3925c4721c5..05ee5df0e95a433 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -647,26 +647,26 @@ TEST(TransferTest, SelfReferentialPointerVarDecl) {
const auto &FooLoc =
*cast(Env.getStorageLocation(*FooDecl));
const auto &FooVal = *cast(Env.getValue(FooLoc));
- const auto &FooPointeeVal =
- *cast(Env.getValue(FooVal.getPointeeLoc()));
+ const auto &FooPointeeLoc =
+ cast(FooVal.getPointeeLoc());
const auto &BarVal =
- *cast(getFieldValue(&FooPointeeVal, *BarDecl, Env));
- const auto &BarPointeeVal =
- *cast(Env.getValue(BarVal.getPointeeLoc()));
+ *cast(getFieldValue(&FooPointeeLoc, *BarDecl, Env));
+ const auto &BarPointeeLoc =
+ cast(BarVal.getPointeeLoc());
- EXPECT_THAT(getFieldValue(&BarPointeeVal, *FooRefDecl, Env), NotNull());
+ EXPECT_THAT(getFieldValue(&BarPointeeLoc, *FooRefDecl, Env), NotNull());
const auto &FooPtrVal = *cast(
- getFieldValue(&BarPointeeVal, *FooPtrDecl, Env));
+ getFieldValue(&BarPointeeLoc, *FooPtrDecl, Env));
const auto &FooPtrPointeeLoc =
cast(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(
- 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(Env.getValue(*FooDecl));
+ const auto &FooLoc =
+ *cast(Env.getStorageLocation(*FooDecl));
const auto *BarVal = cast(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(Env.getValue(*BarArgDecl));
const auto *QuxArgVal = cast(Env.getValue(*QuxArgDecl));
- const auto *QuuxVal = cast(Env.getValue(*QuuxDecl));
- ASSERT_THAT(QuuxVal, NotNull());
-
- const auto *BazVal =
- cast(getFieldValue(QuuxVal, *BazDecl, Env));
- ASSERT_THAT(BazVal, NotNull());
+ const auto &QuuxLoc =
+ *cast(Env.getStorageLocation(*QuuxDecl));
+ const auto &BazLoc =
+ *cast(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(Env.getValue(*BarDecl));
- const auto *A2Val = cast(Env.getValue(*A2Decl));
- EXPECT_EQ(getFieldValue(A2Val, *FooDecl, Env), BarVal);
+ const auto &A2Loc =
+ *cast(Env.getStorageLocation(*A2Decl));
+ EXPECT_EQ(getFieldValue(&A2Loc, *FooDecl, Env), BarVal);
});
}
|
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to eliminate the
RecordStorageLocationfromRecordValueand,ultimately, eliminate
RecordValueentirely (see the discussion linked in theRecordValueclass comment). This is one step in that direction.To eliminate
RecordValue::getChild(), we also eliminate the last remainingcaller, namely the
getFieldValue(const RecordValue *, ...)overload. Callsto this overload have been rewritten to use the
getFieldValue(const RecordStorageLocation *, ...)overload. Note that thisalso makes the code slightly simpler in many cases.