r184313 - Completely revamp node binding for AST matchers.
Manuel Klimek
klimek at google.com
Wed Jun 19 08:42:46 PDT 2013
Author: klimek
Date: Wed Jun 19 10:42:45 2013
New Revision: 184313
URL: http://llvm.org/viewvc/llvm-project?rev=184313&view=rev
Log:
Completely revamp node binding for AST matchers.
This is in preparation for the backwards references to bound
nodes, which will expose a lot more about how matches occur. Main
changes:
- instead of building the tree of bound nodes, we build a "set" of bound
nodes and explode all possible match combinations while running
through the matchers; this will allow us to also implement matchers
that filter down the current set of matches, like "equalsBoundNode"
- take the set of bound nodes at the start of the match into
consideration when doing memoization; as part of that, reevaluated
that memoization gives us benefits that are large enough (it still
does - the effect on common match patterns is up to an order of
magnitude)
- reset the bound nodes when a node does not match, thus never leaking
information from partial sub-matcher matches for failing matchers
Effects:
- we can now correctly "explode" combinatorial matches, for example:
allOf(forEachDescendant(...bind("a")),
forEachDescendant(...bind("b"))) will now trigger matches for all
combinations of matching "a" and "b"s.
- we now never expose bound nodes from partial matches in matchers that
did not match in the end - this fixes a long-standing issue
FIXMEs:
- rename BoundNodesTreeBuilder to BoundNodesBuilder or
BoundNodesSetBuilder, as we don't build a tree any more; this is out
of scope for this change, though
- we're seeing some performance regressions (around 10%), but I expect
some performance tuning will get that back, and it's easily worth
the increase in expressiveness for now
Modified:
cfe/trunk/include/clang/AST/ASTTypeTraits.h
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
Modified: cfe/trunk/include/clang/AST/ASTTypeTraits.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTTypeTraits.h?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/ASTTypeTraits.h (original)
+++ cfe/trunk/include/clang/AST/ASTTypeTraits.h Wed Jun 19 10:42:45 2013
@@ -67,6 +67,25 @@ public:
/// method returns NULL.
const void *getMemoizationData() const;
+ /// @{
+ /// \brief Imposes an order on \c DynTypedNode.
+ ///
+ /// Supports comparison of nodes that support memoization.
+ /// FIXME: Implement comparsion for other node types (currently
+ /// only Stmt and Decl return memoization data).
+ bool operator<(const DynTypedNode &Other) const {
+ assert(getMemoizationData() && Other.getMemoizationData());
+ return getMemoizationData() < Other.getMemoizationData();
+ }
+ bool operator==(const DynTypedNode &Other) const {
+ assert(getMemoizationData() && Other.getMemoizationData());
+ return getMemoizationData() == Other.getMemoizationData();
+ }
+ bool operator!=(const DynTypedNode &Other) const {
+ return !operator==(Other);
+ }
+ /// @}
+
private:
/// \brief Takes care of converting from and to \c T.
template <typename T, typename EnablerT = void> struct BaseConverter;
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Wed Jun 19 10:42:45 2013
@@ -92,7 +92,7 @@ private:
internal::BoundNodesMap MyBoundNodes;
- friend class internal::BoundNodesTree;
+ friend class internal::BoundNodesTreeBuilder;
};
/// \brief If the provided matcher matches a node, binds the node to \c ID.
@@ -281,12 +281,9 @@ AST_MATCHER(Decl, isPrivate) {
/// matches the specialization \c A<int>
AST_MATCHER_P(ClassTemplateSpecializationDecl, hasAnyTemplateArgument,
internal::Matcher<TemplateArgument>, InnerMatcher) {
- const TemplateArgumentList &List = Node.getTemplateArgs();
- for (unsigned i = 0; i < List.size(); ++i) {
- if (InnerMatcher.matches(List.get(i), Finder, Builder))
- return true;
- }
- return false;
+ llvm::ArrayRef<TemplateArgument> List = Node.getTemplateArgs().asArray();
+ return matchesFirstInRange(InnerMatcher, List.begin(), List.end(), Finder,
+ Builder);
}
/// \brief Matches expressions that match InnerMatcher after any implicit casts
@@ -1498,12 +1495,8 @@ inline internal::Matcher<CXXRecordDecl>
/// but not \c B.
AST_MATCHER_P(CXXRecordDecl, hasMethod, internal::Matcher<CXXMethodDecl>,
InnerMatcher) {
- for (CXXRecordDecl::method_iterator I = Node.method_begin(),
- E = Node.method_end();
- I != E; ++I)
- if (InnerMatcher.matches(**I, Finder, Builder))
- return true;
- return false;
+ return matchesFirstInPointerRange(InnerMatcher, Node.method_begin(),
+ Node.method_end(), Finder, Builder);
}
/// \brief Matches AST nodes that have child AST nodes that match the
@@ -2066,13 +2059,8 @@ AST_MATCHER_P2(DeclStmt, containsDeclara
/// record matches Foo, hasAnyConstructorInitializer matches foo_(1)
AST_MATCHER_P(CXXConstructorDecl, hasAnyConstructorInitializer,
internal::Matcher<CXXCtorInitializer>, InnerMatcher) {
- for (CXXConstructorDecl::init_const_iterator I = Node.init_begin();
- I != Node.init_end(); ++I) {
- if (InnerMatcher.matches(**I, Finder, Builder)) {
- return true;
- }
- }
- return false;
+ return matchesFirstInPointerRange(InnerMatcher, Node.init_begin(),
+ Node.init_end(), Finder, Builder);
}
/// \brief Matches the field declaration of a constructor initializer.
@@ -2149,6 +2137,11 @@ AST_MATCHER(CXXConstructorDecl, isImplic
/// matches x(1, y, 42)
/// with hasAnyArgument(...)
/// matching y
+///
+/// FIXME: Currently this will ignore parentheses and implicit casts on
+/// the argument before applying the inner matcher. We'll want to remove
+/// this to allow for greater control by the user once \c ignoreImplicit()
+/// has been implemented.
AST_POLYMORPHIC_MATCHER_P(hasAnyArgument, internal::Matcher<Expr>,
InnerMatcher) {
TOOLING_COMPILE_ASSERT((llvm::is_base_of<CallExpr, NodeType>::value ||
@@ -2156,8 +2149,10 @@ AST_POLYMORPHIC_MATCHER_P(hasAnyArgument
NodeType>::value),
instantiated_with_wrong_types);
for (unsigned I = 0; I < Node.getNumArgs(); ++I) {
- if (InnerMatcher.matches(*Node.getArg(I)->IgnoreParenImpCasts(),
- Finder, Builder)) {
+ BoundNodesTreeBuilder Result(*Builder);
+ if (InnerMatcher.matches(*Node.getArg(I)->IgnoreParenImpCasts(), Finder,
+ &Result)) {
+ *Builder = Result;
return true;
}
}
@@ -2196,12 +2191,8 @@ AST_MATCHER_P2(FunctionDecl, hasParamete
/// matching int y
AST_MATCHER_P(FunctionDecl, hasAnyParameter,
internal::Matcher<ParmVarDecl>, InnerMatcher) {
- for (unsigned I = 0; I < Node.getNumParams(); ++I) {
- if (InnerMatcher.matches(*Node.getParamDecl(I), Finder, Builder)) {
- return true;
- }
- }
- return false;
+ return matchesFirstInPointerRange(InnerMatcher, Node.param_begin(),
+ Node.param_end(), Finder, Builder);
}
/// \brief Matches \c FunctionDecls that have a specific parameter count.
@@ -2350,12 +2341,8 @@ AST_POLYMORPHIC_MATCHER_P(hasBody, inter
/// matching '{}'
AST_MATCHER_P(CompoundStmt, hasAnySubstatement,
internal::Matcher<Stmt>, InnerMatcher) {
- for (CompoundStmt::const_body_iterator It = Node.body_begin();
- It != Node.body_end();
- ++It) {
- if (InnerMatcher.matches(**It, Finder, Builder)) return true;
- }
- return false;
+ return matchesFirstInPointerRange(InnerMatcher, Node.body_begin(),
+ Node.body_end(), Finder, Builder);
}
/// \brief Checks that a compound statement contains a specific number of
@@ -2716,12 +2703,8 @@ AST_MATCHER_P(MemberExpr, hasObjectExpre
/// matches \code using X::b \endcode
AST_MATCHER_P(UsingDecl, hasAnyUsingShadowDecl,
internal::Matcher<UsingShadowDecl>, InnerMatcher) {
- for (UsingDecl::shadow_iterator II = Node.shadow_begin();
- II != Node.shadow_end(); ++II) {
- if (InnerMatcher.matches(**II, Finder, Builder))
- return true;
- }
- return false;
+ return matchesFirstInPointerRange(InnerMatcher, Node.shadow_begin(),
+ Node.shadow_end(), Finder, Builder);
}
/// \brief Matches a using shadow declaration where the target declaration is
@@ -3388,6 +3371,7 @@ AST_MATCHER_P_OVERLOAD(Stmt, equalsNode,
/// "switch (1)", "switch (2)" and "switch (2)".
AST_MATCHER_P(SwitchStmt, forEachSwitchCase, internal::Matcher<SwitchCase>,
InnerMatcher) {
+ BoundNodesTreeBuilder Result;
// FIXME: getSwitchCaseList() does not necessarily guarantee a stable
// iteration order. We should use the more general iterating matchers once
// they are capable of expressing this matcher (for example, it should ignore
@@ -3395,14 +3379,14 @@ AST_MATCHER_P(SwitchStmt, forEachSwitchC
bool Matched = false;
for (const SwitchCase *SC = Node.getSwitchCaseList(); SC;
SC = SC->getNextSwitchCase()) {
- BoundNodesTreeBuilder CaseBuilder;
+ BoundNodesTreeBuilder CaseBuilder(*Builder);
bool CaseMatched = InnerMatcher.matches(*SC, Finder, &CaseBuilder);
if (CaseMatched) {
Matched = true;
- Builder->addMatch(CaseBuilder.build());
+ Result.addMatch(CaseBuilder);
}
}
-
+ *Builder = Result;
return Matched;
}
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Wed Jun 19 10:42:45 2013
@@ -60,7 +60,6 @@ class BoundNodes;
namespace internal {
-class BoundNodesTreeBuilder;
/// \brief Internal version of BoundNodes. Holds all the bound nodes.
class BoundNodesMap {
public:
@@ -71,9 +70,6 @@ public:
void addNode(StringRef ID, const T* Node) {
NodeMap[ID] = ast_type_traits::DynTypedNode::create(*Node);
}
- void addNode(StringRef ID, ast_type_traits::DynTypedNode Node) {
- NodeMap[ID] = Node;
- }
/// \brief Returns the AST node bound to \c ID.
///
@@ -88,29 +84,27 @@ public:
return It->second.get<T>();
}
- /// \brief Copies all ID/Node pairs to BoundNodesTreeBuilder \c Builder.
- void copyTo(BoundNodesTreeBuilder *Builder) const;
-
- /// \brief Copies all ID/Node pairs to BoundNodesMap \c Other.
- void copyTo(BoundNodesMap *Other) const;
+ /// \brief Imposes an order on BoundNodesMaps.
+ bool operator<(const BoundNodesMap &Other) const {
+ return NodeMap < Other.NodeMap;
+ }
private:
/// \brief A map from IDs to the bound nodes.
+ ///
+ /// Note that we're using std::map here, as for memoization:
+ /// - we need a comparison operator
+ /// - we need an assignment operator
typedef std::map<std::string, ast_type_traits::DynTypedNode> IDToNodeMap;
IDToNodeMap NodeMap;
};
-/// \brief A tree of bound nodes in match results.
+/// \brief Creates BoundNodesTree objects.
///
-/// If a match can contain multiple matches on the same node with different
-/// matching subexpressions, BoundNodesTree contains a branch for each of
-/// those matching subexpressions.
-///
-/// BoundNodesTree's are created during the matching process; when a match
-/// is found, we iterate over the tree and create a BoundNodes object containing
-/// the union of all bound nodes on the path from the root to a each leaf.
-class BoundNodesTree {
+/// The tree builder is used during the matching process to insert the bound
+/// nodes from the Id matcher.
+class BoundNodesTreeBuilder {
public:
/// \brief A visitor interface to visit all BoundNodes results for a
/// BoundNodesTree.
@@ -124,63 +118,29 @@ public:
virtual void visitMatch(const BoundNodes& BoundNodesView) = 0;
};
- BoundNodesTree();
-
- /// \brief Create a BoundNodesTree from pre-filled maps of bindings.
- BoundNodesTree(const BoundNodesMap& Bindings,
- const std::vector<BoundNodesTree> RecursiveBindings);
+ /// \brief Add a binding from an id to a node.
+ template <typename T> void setBinding(const std::string &Id, const T *Node) {
+ if (Bindings.empty())
+ Bindings.push_back(BoundNodesMap());
+ for (unsigned i = 0, e = Bindings.size(); i != e; ++i)
+ Bindings[i].addNode(Id, Node);
+ }
- /// \brief Adds all bound nodes to \c Builder.
- void copyTo(BoundNodesTreeBuilder* Builder) const;
+ /// \brief Adds a branch in the tree.
+ void addMatch(const BoundNodesTreeBuilder &Bindings);
/// \brief Visits all matches that this BoundNodesTree represents.
///
/// The ownership of 'ResultVisitor' remains at the caller.
void visitMatches(Visitor* ResultVisitor);
-private:
- void visitMatchesRecursively(
- Visitor* ResultVistior,
- const BoundNodesMap& AggregatedBindings);
-
- // FIXME: Find out whether we want to use different data structures here -
- // first benchmarks indicate that it doesn't matter though.
-
- BoundNodesMap Bindings;
-
- std::vector<BoundNodesTree> RecursiveBindings;
-};
-
-/// \brief Creates BoundNodesTree objects.
-///
-/// The tree builder is used during the matching process to insert the bound
-/// nodes from the Id matcher.
-class BoundNodesTreeBuilder {
-public:
- BoundNodesTreeBuilder();
-
- /// \brief Add a binding from an id to a node.
- template <typename T>
- void setBinding(const std::string &Id, const T *Node) {
- Bindings.addNode(Id, Node);
- }
- void setBinding(const std::string &Id, ast_type_traits::DynTypedNode Node) {
- Bindings.addNode(Id, Node);
+ /// \brief Imposes an order on BoundNodesTreeBuilders.
+ bool operator<(const BoundNodesTreeBuilder &Other) const {
+ return Bindings < Other.Bindings;
}
- /// \brief Adds a branch in the tree.
- void addMatch(const BoundNodesTree& Bindings);
-
- /// \brief Returns a BoundNodes object containing all current bindings.
- BoundNodesTree build() const;
-
private:
- BoundNodesTreeBuilder(const BoundNodesTreeBuilder &) LLVM_DELETED_FUNCTION;
- void operator=(const BoundNodesTreeBuilder &) LLVM_DELETED_FUNCTION;
-
- BoundNodesMap Bindings;
-
- std::vector<BoundNodesTree> RecursiveBindings;
+ SmallVector<BoundNodesMap, 16> Bindings;
};
class ASTMatchFinder;
@@ -290,7 +250,13 @@ public:
bool matches(const T &Node,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- return Implementation->matches(Node, Finder, Builder);
+ if (Implementation->matches(Node, Finder, Builder))
+ return true;
+ // Delete all bindings when a matcher does not match.
+ // This prevents unexpected exposure of bound nodes in unmatches
+ // branches of the match tree.
+ *Builder = BoundNodesTreeBuilder();
+ return false;
}
/// \brief Returns an ID that uniquely identifies the matcher.
@@ -364,6 +330,37 @@ inline Matcher<T> makeMatcher(MatcherInt
return Matcher<T>(Implementation);
}
+/// \brief Finds the first node in a range that matches the given matcher.
+template <typename MatcherT, typename IteratorT>
+bool matchesFirstInRange(const MatcherT &Matcher, IteratorT Start,
+ IteratorT End, ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) {
+ for (IteratorT I = Start; I != End; ++I) {
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Matcher.matches(*I, Finder, &Result)) {
+ *Builder = Result;
+ return true;
+ }
+ }
+ return false;
+}
+
+/// \brief Finds the first node in a pointer range that matches the given
+/// matcher.
+template <typename MatcherT, typename IteratorT>
+bool matchesFirstInPointerRange(const MatcherT &Matcher, IteratorT Start,
+ IteratorT End, ASTMatchFinder *Finder,
+ BoundNodesTreeBuilder *Builder) {
+ for (IteratorT I = Start; I != End; ++I) {
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Matcher.matches(**I, Finder, &Result)) {
+ *Builder = Result;
+ return true;
+ }
+ }
+ return false;
+}
+
/// \brief Metafunction to determine if type T has a member called getDecl.
template <typename T> struct has_getDecl {
struct Default { int getDecl; };
@@ -888,7 +885,18 @@ public:
virtual bool matches(const T &Node,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- return !InnerMatcher.matches(Node, Finder, Builder);
+ // The 'unless' matcher will always discard the result:
+ // If the inner matcher doesn't match, unless returns true,
+ // but the inner matcher cannot have bound anything.
+ // If the inner matcher matches, the result is false, and
+ // any possible binding will be discarded.
+ // We still need to hand in all the bound nodes up to this
+ // point so the inner matcher can depend on bound nodes,
+ // and we need to actively discard the bound nodes, otherwise
+ // the inner matcher will reset the bound nodes if it doesn't
+ // match, but this would be inversed by 'unless'.
+ BoundNodesTreeBuilder Discard(*Builder);
+ return !InnerMatcher.matches(Node, Finder, &Discard);
}
private:
@@ -909,6 +917,9 @@ public:
virtual bool matches(const T &Node,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
+ // allOf leads to one matcher for each alternative in the first
+ // matcher combined with each alternative in the second matcher.
+ // Thus, we can reuse the same Builder.
return InnerMatcher1.matches(Node, Finder, Builder) &&
InnerMatcher2.matches(Node, Finder, Builder);
}
@@ -935,16 +946,18 @@ public:
virtual bool matches(const T &Node, ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- BoundNodesTreeBuilder Builder1;
+ BoundNodesTreeBuilder Result;
+ BoundNodesTreeBuilder Builder1(*Builder);
bool Matched1 = InnerMatcher1.matches(Node, Finder, &Builder1);
if (Matched1)
- Builder->addMatch(Builder1.build());
+ Result.addMatch(Builder1);
- BoundNodesTreeBuilder Builder2;
+ BoundNodesTreeBuilder Builder2(*Builder);
bool Matched2 = InnerMatcher2.matches(Node, Finder, &Builder2);
if (Matched2)
- Builder->addMatch(Builder2.build());
+ Result.addMatch(Builder2);
+ *Builder = Result;
return Matched1 || Matched2;
}
@@ -969,8 +982,17 @@ public:
virtual bool matches(const T &Node,
ASTMatchFinder *Finder,
BoundNodesTreeBuilder *Builder) const {
- return InnerMatcher1.matches(Node, Finder, Builder) ||
- InnerMatcher2.matches(Node, Finder, Builder);
+ BoundNodesTreeBuilder Result = *Builder;
+ if (InnerMatcher1.matches(Node, Finder, &Result)) {
+ *Builder = Result;
+ return true;
+ }
+ Result = *Builder;
+ if (InnerMatcher2.matches(Node, Finder, &Result)) {
+ *Builder = Result;
+ return true;
+ }
+ return false;
}
private:
Modified: cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp Wed Jun 19 10:42:45 2013
@@ -30,10 +30,21 @@ namespace {
typedef MatchFinder::MatchCallback MatchCallback;
+// The maximum number of memoization entries to store.
+// 10k has been experimentally found to give a good trade-off
+// of performance vs. memory consumption by running matcher
+// that match on every statement over a very large codebase.
+//
+// FIXME: Do some performance optimization in general and
+// revisit this number; also, put up micro-benchmarks that we can
+// optimize this on.
+static const unsigned MaxMemoizationEntries = 10000;
+
// We use memoization to avoid running the same matcher on the same
-// AST node twice. This pair is the key for looking up match
+// AST node twice. This struct is the key for looking up match
// result. It consists of an ID of the MatcherInterface (for
-// identifying the matcher) and a pointer to the AST node.
+// identifying the matcher), a pointer to the AST node and the
+// bound nodes before the matcher was executed.
//
// We currently only memoize on nodes whose pointers identify the
// nodes (\c Stmt and \c Decl, but not \c QualType or \c TypeLoc).
@@ -41,12 +52,24 @@ typedef MatchFinder::MatchCallback Match
// generation of keys for each type.
// FIXME: Benchmark whether memoization of non-pointer typed nodes
// provides enough benefit for the additional amount of code.
-typedef std::pair<uint64_t, const void*> UntypedMatchInput;
+struct MatchKey {
+ uint64_t MatcherID;
+ ast_type_traits::DynTypedNode Node;
+ BoundNodesTreeBuilder BoundNodes;
+
+ bool operator<(const MatchKey &Other) const {
+ if (MatcherID != Other.MatcherID)
+ return MatcherID < Other.MatcherID;
+ if (Node != Other.Node)
+ return Node < Other.Node;
+ return BoundNodes < Other.BoundNodes;
+ }
+};
// Used to store the result of a match and possibly bound nodes.
struct MemoizedMatchResult {
bool ResultOfMatch;
- BoundNodesTree Nodes;
+ BoundNodesTreeBuilder Nodes;
};
// A RecursiveASTVisitor that traverses all children or all descendants of
@@ -103,6 +126,12 @@ public:
else if (const TypeLoc *T = DynNode.get<TypeLoc>())
traverse(*T);
// FIXME: Add other base types after adding tests.
+
+ // It's OK to always overwrite the bound nodes, as if there was
+ // no match in this recursive branch, the result set is empty
+ // anyway.
+ *Builder = ResultBindings;
+
return Matches;
}
@@ -220,18 +249,20 @@ private:
return true;
}
if (Bind != ASTMatchFinder::BK_All) {
- if (Matcher->matches(ast_type_traits::DynTypedNode::create(Node),
- Finder, Builder)) {
+ BoundNodesTreeBuilder RecursiveBuilder(*Builder);
+ if (Matcher->matches(ast_type_traits::DynTypedNode::create(Node), Finder,
+ &RecursiveBuilder)) {
Matches = true;
- return false; // Abort as soon as a match is found.
+ ResultBindings.addMatch(RecursiveBuilder);
+ return false; // Abort as soon as a match is found.
}
} else {
- BoundNodesTreeBuilder RecursiveBuilder;
- if (Matcher->matches(ast_type_traits::DynTypedNode::create(Node),
- Finder, &RecursiveBuilder)) {
+ BoundNodesTreeBuilder RecursiveBuilder(*Builder);
+ if (Matcher->matches(ast_type_traits::DynTypedNode::create(Node), Finder,
+ &RecursiveBuilder)) {
// After the first match the matcher succeeds.
Matches = true;
- Builder->addMatch(RecursiveBuilder.build());
+ ResultBindings.addMatch(RecursiveBuilder);
}
}
return true;
@@ -251,6 +282,7 @@ private:
const DynTypedMatcher *const Matcher;
ASTMatchFinder *const Finder;
BoundNodesTreeBuilder *const Builder;
+ BoundNodesTreeBuilder ResultBindings;
int CurrentDepth;
const int MaxDepth;
const ASTMatchFinder::TraversalKind Traversal;
@@ -341,24 +373,28 @@ public:
const DynTypedMatcher &Matcher,
BoundNodesTreeBuilder *Builder, int MaxDepth,
TraversalKind Traversal, BindKind Bind) {
- const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
+ MatchKey Key;
+ Key.MatcherID = Matcher.getID();
+ Key.Node = Node;
+ // Note that we key on the bindings *before* the match.
+ Key.BoundNodes = *Builder;
// For AST-nodes that don't have an identity, we can't memoize.
- if (!input.second)
+ if (!Node.getMemoizationData())
return matchesRecursively(Node, Matcher, Builder, MaxDepth, Traversal,
Bind);
- std::pair<MemoizationMap::iterator, bool> InsertResult
- = ResultCache.insert(std::make_pair(input, MemoizedMatchResult()));
+ if (ResultCache.size() > MaxMemoizationEntries)
+ ResultCache.clear();
+ std::pair<MemoizationMap::iterator, bool> InsertResult =
+ ResultCache.insert(std::make_pair(Key, MemoizedMatchResult()));
if (InsertResult.second) {
- BoundNodesTreeBuilder DescendantBoundNodesBuilder;
+ InsertResult.first->second.Nodes = *Builder;
InsertResult.first->second.ResultOfMatch =
- matchesRecursively(Node, Matcher, &DescendantBoundNodesBuilder,
- MaxDepth, Traversal, Bind);
- InsertResult.first->second.Nodes =
- DescendantBoundNodesBuilder.build();
+ matchesRecursively(Node, Matcher, &InsertResult.first->second.Nodes,
+ MaxDepth, Traversal, Bind);
}
- InsertResult.first->second.Nodes.copyTo(Builder);
+ *Builder = InsertResult.first->second.Nodes;
return InsertResult.first->second.ResultOfMatch;
}
@@ -411,9 +447,8 @@ public:
I != E; ++I) {
BoundNodesTreeBuilder Builder;
if (I->first->matches(Node, this, &Builder)) {
- BoundNodesTree BoundNodes = Builder.build();
MatchVisitor Visitor(ActiveASTContext, I->second);
- BoundNodes.visitMatches(&Visitor);
+ Builder.visitMatches(&Visitor);
}
}
}
@@ -459,19 +494,29 @@ private:
assert(false && "Found node that is not in the parent map.");
return false;
}
- const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
- MemoizationMap::iterator I = ResultCache.find(input);
- if (I == ResultCache.end()) {
- BoundNodesTreeBuilder AncestorBoundNodesBuilder;
+ MatchKey Key;
+ Key.MatcherID = Matcher.getID();
+ Key.Node = Node;
+ Key.BoundNodes = *Builder;
+ if (ResultCache.size() > MaxMemoizationEntries)
+ ResultCache.clear();
+ std::pair<MemoizationMap::iterator, bool> InsertResult =
+ ResultCache.insert(std::make_pair(Key, MemoizedMatchResult()));
+ if (InsertResult.second) {
bool Matches = false;
if (Parents.size() == 1) {
// Only one parent - do recursive memoization.
const ast_type_traits::DynTypedNode Parent = Parents[0];
- if (Matcher.matches(Parent, this, &AncestorBoundNodesBuilder)) {
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Matcher.matches(Parent, this, &Result)) {
+ InsertResult.first->second.Nodes = Result;
Matches = true;
} else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
- Matches = memoizedMatchesAncestorOfRecursively(
- Parent, Matcher, &AncestorBoundNodesBuilder, MatchMode);
+ Matches = memoizedMatchesAncestorOfRecursively(Parent, Matcher,
+ Builder, MatchMode);
+ // Once we get back from the recursive call, the result will be the
+ // same as the parent's result.
+ InsertResult.first->second.Nodes = *Builder;
}
} else {
// Multiple parents - BFS over the rest of the nodes.
@@ -479,8 +524,9 @@ private:
std::deque<ast_type_traits::DynTypedNode> Queue(Parents.begin(),
Parents.end());
while (!Queue.empty()) {
- if (Matcher.matches(Queue.front(), this,
- &AncestorBoundNodesBuilder)) {
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Matcher.matches(Queue.front(), this, &Result)) {
+ InsertResult.first->second.Nodes = Result;
Matches = true;
break;
}
@@ -501,18 +547,15 @@ private:
}
}
- I = ResultCache.insert(std::make_pair(input, MemoizedMatchResult()))
- .first;
- I->second.Nodes = AncestorBoundNodesBuilder.build();
- I->second.ResultOfMatch = Matches;
+ InsertResult.first->second.ResultOfMatch = Matches;
}
- I->second.Nodes.copyTo(Builder);
- return I->second.ResultOfMatch;
+ *Builder = InsertResult.first->second.Nodes;
+ return InsertResult.first->second.ResultOfMatch;
}
// Implements a BoundNodesTree::Visitor that calls a MatchCallback with
// the aggregated bound nodes for each match.
- class MatchVisitor : public BoundNodesTree::Visitor {
+ class MatchVisitor : public BoundNodesTreeBuilder::Visitor {
public:
MatchVisitor(ASTContext* Context,
MatchFinder::MatchCallback* Callback)
@@ -538,8 +581,11 @@ private:
for (std::set<const TypedefDecl*>::const_iterator
It = Aliases.begin(), End = Aliases.end();
It != End; ++It) {
- if (Matcher.matches(**It, this, Builder))
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Matcher.matches(**It, this, &Result)) {
+ *Builder = Result;
return true;
+ }
}
return false;
}
@@ -552,7 +598,7 @@ private:
llvm::DenseMap<const Type*, std::set<const TypedefDecl*> > TypeAliases;
// Maps (matcher, node) -> the match result for memoization.
- typedef llvm::DenseMap<UntypedMatchInput, MemoizedMatchResult> MemoizationMap;
+ typedef std::map<MatchKey, MemoizedMatchResult> MemoizationMap;
MemoizationMap ResultCache;
};
@@ -616,8 +662,11 @@ bool MatchASTVisitor::classIsDerivedFrom
assert(TemplateType);
return false;
}
- if (Base.matches(*ClassDecl, this, Builder))
+ BoundNodesTreeBuilder Result(*Builder);
+ if (Base.matches(*ClassDecl, this, &Result)) {
+ *Builder = Result;
return true;
+ }
if (classIsDerivedFrom(ClassDecl, Base, Builder))
return true;
}
Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Wed Jun 19 10:42:45 2013
@@ -18,70 +18,20 @@ namespace clang {
namespace ast_matchers {
namespace internal {
-void BoundNodesMap::copyTo(BoundNodesTreeBuilder *Builder) const {
- for (IDToNodeMap::const_iterator It = NodeMap.begin();
- It != NodeMap.end();
- ++It) {
- Builder->setBinding(It->first, It->second);
+void BoundNodesTreeBuilder::visitMatches(Visitor *ResultVisitor) {
+ if (Bindings.empty())
+ Bindings.push_back(BoundNodesMap());
+ for (unsigned i = 0, e = Bindings.size(); i != e; ++i) {
+ ResultVisitor->visitMatch(BoundNodes(Bindings[i]));
}
}
-void BoundNodesMap::copyTo(BoundNodesMap *Other) const {
- for (IDToNodeMap::const_iterator I = NodeMap.begin(),
- E = NodeMap.end();
- I != E; ++I) {
- Other->NodeMap[I->first] = I->second;
+void BoundNodesTreeBuilder::addMatch(const BoundNodesTreeBuilder &Other) {
+ for (unsigned i = 0, e = Other.Bindings.size(); i != e; ++i) {
+ Bindings.push_back(Other.Bindings[i]);
}
}
-BoundNodesTree::BoundNodesTree() {}
-
-BoundNodesTree::BoundNodesTree(
- const BoundNodesMap& Bindings,
- const std::vector<BoundNodesTree> RecursiveBindings)
- : Bindings(Bindings),
- RecursiveBindings(RecursiveBindings) {}
-
-void BoundNodesTree::copyTo(BoundNodesTreeBuilder* Builder) const {
- Bindings.copyTo(Builder);
- for (std::vector<BoundNodesTree>::const_iterator
- I = RecursiveBindings.begin(),
- E = RecursiveBindings.end();
- I != E; ++I) {
- Builder->addMatch(*I);
- }
-}
-
-void BoundNodesTree::visitMatches(Visitor* ResultVisitor) {
- BoundNodesMap AggregatedBindings;
- visitMatchesRecursively(ResultVisitor, AggregatedBindings);
-}
-
-void BoundNodesTree::
-visitMatchesRecursively(Visitor* ResultVisitor,
- const BoundNodesMap& AggregatedBindings) {
- BoundNodesMap CombinedBindings(AggregatedBindings);
- Bindings.copyTo(&CombinedBindings);
- if (RecursiveBindings.empty()) {
- ResultVisitor->visitMatch(BoundNodes(CombinedBindings));
- } else {
- for (unsigned I = 0; I < RecursiveBindings.size(); ++I) {
- RecursiveBindings[I].visitMatchesRecursively(ResultVisitor,
- CombinedBindings);
- }
- }
-}
-
-BoundNodesTreeBuilder::BoundNodesTreeBuilder() {}
-
-void BoundNodesTreeBuilder::addMatch(const BoundNodesTree& Bindings) {
- RecursiveBindings.push_back(Bindings);
-}
-
-BoundNodesTree BoundNodesTreeBuilder::build() const {
- return BoundNodesTree(Bindings, RecursiveBindings);
-}
-
DynTypedMatcher::~DynTypedMatcher() {}
DynTypedMatcher *DynTypedMatcher::tryBind(StringRef ID) const { return NULL; }
Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=184313&r1=184312&r2=184313&view=diff
==============================================================================
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp (original)
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp Wed Jun 19 10:42:45 2013
@@ -3009,12 +3009,13 @@ TEST(ForEachDescendant, NestedForEachDes
recordDecl(hasName("A"), anyOf(m, forEachDescendant(m))),
new VerifyIdIsBoundTo<Decl>("x", "C")));
- // FIXME: This is not really a useful matcher, but the result is still
- // surprising (currently binds "A").
- //EXPECT_TRUE(matchAndVerifyResultTrue(
- // "class A { class B { class C {}; }; };",
- // recordDecl(hasName("A"), allOf(hasDescendant(m), anyOf(m, anything()))),
- // new VerifyIdIsBoundTo<Decl>("x", "C")));
+ // Check that a partial match of 'm' that binds 'x' in the
+ // first part of anyOf(m, anything()) will not overwrite the
+ // binding created by the earlier binding in the hasDescendant.
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class A { class B { class C {}; }; };",
+ recordDecl(hasName("A"), allOf(hasDescendant(m), anyOf(m, anything()))),
+ new VerifyIdIsBoundTo<Decl>("x", "C")));
}
TEST(ForEachDescendant, BindsMultipleNodes) {
@@ -3034,6 +3035,112 @@ TEST(ForEachDescendant, BindsRecursiveCo
new VerifyIdIsBoundTo<FieldDecl>("f", 8)));
}
+TEST(ForEachDescendant, BindsCombinations) {
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "void f() { if(true) {} if (true) {} while (true) {} if (true) {} while "
+ "(true) {} }",
+ compoundStmt(forEachDescendant(ifStmt().bind("if")),
+ forEachDescendant(whileStmt().bind("while"))),
+ new VerifyIdIsBoundTo<IfStmt>("if", 6)));
+}
+
+TEST(Has, DoesNotDeleteBindings) {
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X { int a; };", recordDecl(decl().bind("x"), has(fieldDecl())),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+}
+
+TEST(LoopingMatchers, DoNotOverwritePreviousMatchResultOnFailure) {
+ // Those matchers cover all the cases where an inner matcher is called
+ // and there is not a 1:1 relationship between the match of the outer
+ // matcher and the match of the inner matcher.
+ // The pattern to look for is:
+ // ... return InnerMatcher.matches(...); ...
+ // In which case no special handling is needed.
+ //
+ // On the other hand, if there are multiple alternative matches
+ // (for example forEach*) or matches might be discarded (for example has*)
+ // the implementation must make sure that the discarded matches do not
+ // affect the bindings.
+ // When new such matchers are added, add a test here that:
+ // - matches a simple node, and binds it as the first thing in the matcher:
+ // recordDecl(decl().bind("x"), hasName("X")))
+ // - uses the matcher under test afterwards in a way that not the first
+ // alternative is matched; for anyOf, that means the first branch
+ // would need to return false; for hasAncestor, it means that not
+ // the direct parent matches the inner matcher.
+
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X { int y; };",
+ recordDecl(
+ recordDecl().bind("x"), hasName("::X"),
+ anyOf(forEachDescendant(recordDecl(hasName("Y"))), anything())),
+ new VerifyIdIsBoundTo<CXXRecordDecl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X {};", recordDecl(recordDecl().bind("x"), hasName("::X"),
+ anyOf(unless(anything()), anything())),
+ new VerifyIdIsBoundTo<CXXRecordDecl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "template<typename T1, typename T2> class X {}; X<float, int> x;",
+ classTemplateSpecializationDecl(
+ decl().bind("x"),
+ hasAnyTemplateArgument(refersToType(asString("int")))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X { void f(); void g(); };",
+ recordDecl(decl().bind("x"), hasMethod(hasName("g"))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X { X() : a(1), b(2) {} double a; int b; };",
+ recordDecl(decl().bind("x"),
+ has(constructorDecl(
+ hasAnyConstructorInitializer(forField(hasName("b")))))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "void x(int, int) { x(0, 42); }",
+ callExpr(expr().bind("x"), hasAnyArgument(integerLiteral(equals(42)))),
+ new VerifyIdIsBoundTo<Expr>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "void x(int, int y) {}",
+ functionDecl(decl().bind("x"), hasAnyParameter(hasName("y"))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "void x() { return; if (true) {} }",
+ functionDecl(decl().bind("x"),
+ has(compoundStmt(hasAnySubstatement(ifStmt())))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "namespace X { void b(int); void b(); }"
+ "using X::b;",
+ usingDecl(decl().bind("x"), hasAnyUsingShadowDecl(hasTargetDecl(
+ functionDecl(parameterCountIs(1))))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class A{}; class B{}; class C : B, A {};",
+ recordDecl(decl().bind("x"), isDerivedFrom("::A")),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class A{}; typedef A B; typedef A C; typedef A D;"
+ "class E : A {};",
+ recordDecl(decl().bind("x"), isDerivedFrom("C")),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class A { class B { void f() {} }; };",
+ functionDecl(decl().bind("x"), hasAncestor(recordDecl(hasName("::A")))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "template <typename T> struct A { struct B {"
+ " void f() { if(true) {} }"
+ "}; };"
+ "void t() { A<int>::B b; b.f(); }",
+ ifStmt(stmt().bind("x"), hasAncestor(recordDecl(hasName("::A")))),
+ new VerifyIdIsBoundTo<Stmt>("x", 2)));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class A {};",
+ recordDecl(hasName("::A"), decl().bind("x"), unless(hasName("fooble"))),
+ new VerifyIdIsBoundTo<Decl>("x", 1)));
+}
+
TEST(ForEachDescendant, BindsCorrectNodes) {
EXPECT_TRUE(matchAndVerifyResultTrue(
"class C { void f(); int i; };",
More information about the cfe-commits
mailing list