loic-joly-sonarsource created this revision.
loic-joly-sonarsource added a reviewer: klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
In ASTMatcher, when we have `has(...)` and `hasParent(...)` called with the
same internal matcher on the same node, the memoization process will mix-up the
two calls because the direction of the traversal is not part of the memoization
key.
This patch adds this information.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80025
Files:
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2591,6 +2591,14 @@
compoundStmt(hasParent(ifStmt()))));
}
+TEST(MatcherMemoize, HasParentDiffersFromHas) {
+ // Test introduced after detecting a bug in memoization
+ EXPECT_TRUE(matches(
+ "void f() { throw 1; }",
+ expr(eachOf(cxxThrowExpr(hasParent(expr())),
+ cxxThrowExpr(has(expr()))))));
+}
+
TEST(HasAncestor, MatchesAllAncestors) {
EXPECT_TRUE(matches(
"template <typename T> struct C { static void f() { 42; } };"
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -43,6 +43,11 @@
// optimize this on.
static const unsigned MaxMemoizationEntries = 10000;
+enum class MatchDirection {
+ Ancestors,
+ Descendants
+};
+
// We use memoization to avoid running the same matcher on the same
// AST node twice. This struct is the key for looking up match
// result. It consists of an ID of the MatcherInterface (for
@@ -60,11 +65,12 @@
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
ast_type_traits::TraversalKind Traversal = ast_type_traits::TK_AsIs;
+ MatchDirection Direction;
bool operator<(const MatchKey &Other) const {
- return std::tie(MatcherID, Node, BoundNodes, Traversal) <
+ return std::tie(MatcherID, Node, BoundNodes, Traversal, Direction) <
std::tie(Other.MatcherID, Other.Node, Other.BoundNodes,
- Other.Traversal);
+ Other.Traversal, Other.Direction);
}
};
@@ -457,6 +463,7 @@
// Note that we key on the bindings *before* the match.
Key.BoundNodes = *Builder;
Key.Traversal = Ctx.getTraversalKind();
+ Key.Direction = MatchDirection::Descendants;
MemoizationMap::iterator I = ResultCache.find(Key);
if (I != ResultCache.end()) {
@@ -706,6 +713,7 @@
Key.Node = Node;
Key.BoundNodes = *Builder;
Key.Traversal = Ctx.getTraversalKind();
+ Key.Direction = MatchDirection::Ancestors;
// Note that we cannot use insert and reuse the iterator, as recursive
// calls to match might invalidate the result cache iterators.
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2591,6 +2591,14 @@
compoundStmt(hasParent(ifStmt()))));
}
+TEST(MatcherMemoize, HasParentDiffersFromHas) {
+ // Test introduced after detecting a bug in memoization
+ EXPECT_TRUE(matches(
+ "void f() { throw 1; }",
+ expr(eachOf(cxxThrowExpr(hasParent(expr())),
+ cxxThrowExpr(has(expr()))))));
+}
+
TEST(HasAncestor, MatchesAllAncestors) {
EXPECT_TRUE(matches(
"template <typename T> struct C { static void f() { 42; } };"
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -43,6 +43,11 @@
// optimize this on.
static const unsigned MaxMemoizationEntries = 10000;
+enum class MatchDirection {
+ Ancestors,
+ Descendants
+};
+
// We use memoization to avoid running the same matcher on the same
// AST node twice. This struct is the key for looking up match
// result. It consists of an ID of the MatcherInterface (for
@@ -60,11 +65,12 @@
ast_type_traits::DynTypedNode Node;
BoundNodesTreeBuilder BoundNodes;
ast_type_traits::TraversalKind Traversal = ast_type_traits::TK_AsIs;
+ MatchDirection Direction;
bool operator<(const MatchKey &Other) const {
- return std::tie(MatcherID, Node, BoundNodes, Traversal) <
+ return std::tie(MatcherID, Node, BoundNodes, Traversal, Direction) <
std::tie(Other.MatcherID, Other.Node, Other.BoundNodes,
- Other.Traversal);
+ Other.Traversal, Other.Direction);
}
};
@@ -457,6 +463,7 @@
// Note that we key on the bindings *before* the match.
Key.BoundNodes = *Builder;
Key.Traversal = Ctx.getTraversalKind();
+ Key.Direction = MatchDirection::Descendants;
MemoizationMap::iterator I = ResultCache.find(Key);
if (I != ResultCache.end()) {
@@ -706,6 +713,7 @@
Key.Node = Node;
Key.BoundNodes = *Builder;
Key.Traversal = Ctx.getTraversalKind();
+ Key.Direction = MatchDirection::Ancestors;
// Note that we cannot use insert and reuse the iterator, as recursive
// calls to match might invalidate the result cache iterators.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
