https://github.com/steakhal approved this pull request.
This looks pretty good!
Initially I wanted to cleanup the way calculating the levels for the types
because it has some raw loops, variable mutation, non-structured control-flow
aka. breaks etc. But the alternatives I came up with were just more complicated
- albeit more declarative. So decided to not pursuing that direction.
Then I shifted my attention to the unittest where you have multiple projections
with materialization.
We do a poor job on using what gtest and gmock has to offer for declarative
testing and this is a place where it can actually help some. Here is what I'd
apply:
```diff
diff --git
a/clang/unittests/ScalableStaticAnalysis/Analyses/EntityPointerLevel/EntityPointerLevelTest.cpp
b/clang/unittests/ScalableStaticAnalysis/Analyses/EntityPointerLevel/EntityPointerLevelTest.cpp
index d9b5a347edb6..b254d810e349 100644
---
a/clang/unittests/ScalableStaticAnalysis/Analyses/EntityPointerLevel/EntityPointerLevelTest.cpp
+++
b/clang/unittests/ScalableStaticAnalysis/Analyses/EntityPointerLevel/EntityPointerLevelTest.cpp
@@ -14,19 +14,62 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <memory>
-#include <vector>
+#include <ostream>
using namespace clang;
using namespace ssaf;
+using testing::AllOf;
+using testing::Each;
using testing::ElementsAre;
+using testing::Field;
+using testing::Matcher;
+
+namespace clang::ssaf {
+// Let gmock print DeclPointerLevels by declaration name. Found by ADL, so it
+// must live in the namespace of `DeclPointerLevel`.
+void PrintTo(const DeclPointerLevel &DPL, std::ostream *OS) {
+ *OS << "DeclPointerLevel { Decl: '"
+ << (DPL.Decl ? DPL.Decl->getNameAsString() : "<null>")
+ << "', PointerLevel: " << DPL.PointerLevel
+ << ", IsReturn: " << (DPL.IsReturn ? "true" : "false") << " }";
+}
+} // namespace clang::ssaf
namespace {
-static std::vector<unsigned> levelsOf(const DeclPointerLevels &DPLs) {
- std::vector<unsigned> Levels;
- for (const DeclPointerLevel &DPL : DPLs)
- Levels.push_back(DPL.PointerLevel);
- return Levels;
+/// Matches a DeclPointerLevel at pointer level \p Level.
+Matcher<const DeclPointerLevel &> hasPointerLevel(unsigned Level) {
+ return Field("PointerLevel", &DeclPointerLevel::PointerLevel, Level);
+}
+
+/// Matches a DeclPointerLevel of \p ND, for the entity kind selected by
+/// \p IsReturn.
+Matcher<const DeclPointerLevel &> isLevelOfDecl(const NamedDecl *ND,
+ bool IsReturn) {
+ return AllOf(Field("Decl", &DeclPointerLevel::Decl, ND),
+ Field("IsReturn", &DeclPointerLevel::IsReturn, IsReturn));
+}
+
+/// Matches exactly the given pointer levels, in order.
+template <typename... Levels> auto hasPointerLevels(Levels... Ls) {
+ return ElementsAre(hasPointerLevel(Ls)...);
+}
+
+/// Elaborates the DeclPointerLevel of the \p DeclT named \p Name in \p Ctx,
+/// starting at pointer level \p StartLevel, and checks that every result
+/// belongs to that declaration.
+template <typename DeclT = NamedDecl>
+DeclPointerLevels elaborateFor(ASTContext &Ctx, StringRef Name,
+ unsigned StartLevel, bool IsReturn = false) {
+ const DeclT *ND = findDeclByName<DeclT>(Name, Ctx);
+ EXPECT_TRUE(ND) << "decl not found: " << Name.str();
+ if (!ND)
+ return {};
+
+ DeclPointerLevels DPLs = elaborateHigherDeclPointerLevels(
+ DeclPointerLevel{ND, StartLevel, IsReturn});
+ EXPECT_THAT(DPLs, Each(isLevelOfDecl(ND, IsReturn)));
+ return DPLs;
}
// `elaborateHigherDeclPointerLevels` expands a DeclPointerLevel into an
@@ -39,34 +82,24 @@ TEST(EntityPointerLevelTest,
ElaborateHigherDeclPointerLevels) {
int **q; // two pointer levels
int ***r; // three pointer levels
int *arr[10]; // array of pointers: two levels (array + pointer)
+ int mat[3][4]; // 2-D array: two levels
+ typedef int *IP;
+ IP *pp; // sugar for `int **`: two levels
)cpp",
{"-Wno-unused"});
ASSERT_TRUE(AST);
ASTContext &Ctx = AST->getASTContext();
- // Elaborate the decl named `Name` starting at `StartLevel`, checking that
- // every result shares the input's declaration and is-return flag, and return
- // the produced pointer levels.
- auto elaborate = [&](StringRef Name, unsigned StartLevel) {
- const NamedDecl *ND = findDeclByName(Name, Ctx);
- EXPECT_NE(ND, nullptr) << "decl not found: " << Name.str();
- if (!ND)
- return std::vector<unsigned>{};
- DeclPointerLevels DPLs = elaborateHigherDeclPointerLevels(
- DeclPointerLevel{ND, StartLevel, /*IsReturn=*/false});
- for (const DeclPointerLevel &DPL : DPLs) {
- EXPECT_TRUE(DPL.Decl == ND);
- EXPECT_FALSE(DPL.IsReturn);
- }
- return levelsOf(DPLs);
- };
-
- EXPECT_THAT(elaborate("p", 1), ElementsAre(1U)); // int*
- EXPECT_THAT(elaborate("q", 1), ElementsAre(1U, 2U)); // int**
- EXPECT_THAT(elaborate("r", 1), ElementsAre(1U, 2U, 3U)); // int***
- EXPECT_THAT(elaborate("arr", 1), ElementsAre(1U, 2U)); // int*[10]
- EXPECT_THAT(elaborate("r", 2), ElementsAre(2U, 3U));
- EXPECT_THAT(elaborate("r", 3), ElementsAre(3U));
+ EXPECT_THAT(elaborateFor(Ctx, "p", 1), hasPointerLevels(1)); // int*
+ EXPECT_THAT(elaborateFor(Ctx, "q", 1), hasPointerLevels(1, 2)); // int**
+ EXPECT_THAT(elaborateFor(Ctx, "r", 1), hasPointerLevels(1, 2, 3)); // int***
+ EXPECT_THAT(elaborateFor(Ctx, "arr", 1), hasPointerLevels(1, 2)); //
int*[10]
+ EXPECT_THAT(elaborateFor(Ctx, "mat", 1), hasPointerLevels(1, 2)); //
int[3][4]
+ EXPECT_THAT(elaborateFor(Ctx, "pp", 1), hasPointerLevels(1, 2)); // IP*
+ EXPECT_THAT(elaborateFor(Ctx, "r", 2), hasPointerLevels(2, 3));
+ EXPECT_THAT(elaborateFor(Ctx, "r", 3), hasPointerLevels(3));
+ // A level beyond the declared type's level count is preserved as is:
+ EXPECT_THAT(elaborateFor(Ctx, "p", 2), hasPointerLevels(2));
}
// For a function entity (IsReturn=true), the maximum pointer level is bounded
@@ -81,22 +114,10 @@ TEST(EntityPointerLevelTest,
ElaborateHigherDeclPointerLevelsForReturn) {
ASSERT_TRUE(AST);
ASTContext &Ctx = AST->getASTContext();
- auto elaborateReturn = [&](StringRef Name, unsigned StartLevel) {
- const FunctionDecl *FD = findFnByName(Name, Ctx);
- EXPECT_NE(FD, nullptr) << "function not found: " << Name.str();
- if (!FD)
- return std::vector<unsigned>{};
- DeclPointerLevels DPLs = elaborateHigherDeclPointerLevels(
- DeclPointerLevel{FD, StartLevel, /*IsReturn=*/true});
- for (const DeclPointerLevel &DPL : DPLs) {
- EXPECT_TRUE(DPL.Decl == FD);
- EXPECT_TRUE(DPL.IsReturn);
- }
- return levelsOf(DPLs);
- };
-
- EXPECT_THAT(elaborateReturn("refret", 1), ElementsAre(1U, 2U)); // int**&
- EXPECT_THAT(elaborateReturn("valret", 1), ElementsAre(1U)); // int*
+ EXPECT_THAT(elaborateFor<FunctionDecl>(Ctx, "refret", 1, /*IsReturn=*/true),
+ hasPointerLevels(1, 2)); // int**&
+ EXPECT_THAT(elaborateFor<FunctionDecl>(Ctx, "valret", 1, /*IsReturn=*/true),
+ hasPointerLevels(1)); // int*
}
} // namespace
```
I don't think this is a great improvement so I'll let you decide what to adopt
from it if any. I've left some comments inline about the things I'd like to
change in case you don't want to apply the overlapping parts from what I
sketched here.
https://github.com/llvm/llvm-project/pull/218196
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
