From: Manuel Klimek Date: Wed, 19 Jun 2013 15:42:45 +0000 (+0000) Subject: Completely revamp node binding for AST matchers. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=054d049174eb1ec8e93a4a0831c0d8caac00cb3a;p=clang 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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@184313 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/ASTTypeTraits.h b/include/clang/AST/ASTTypeTraits.h index 4688b12de7..1139e0e185 100644 --- a/include/clang/AST/ASTTypeTraits.h +++ b/include/clang/AST/ASTTypeTraits.h @@ -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 struct BaseConverter; diff --git a/include/clang/ASTMatchers/ASTMatchers.h b/include/clang/ASTMatchers/ASTMatchers.h index a66f3afe00..dede68961f 100644 --- a/include/clang/ASTMatchers/ASTMatchers.h +++ b/include/clang/ASTMatchers/ASTMatchers.h @@ -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 AST_MATCHER_P(ClassTemplateSpecializationDecl, hasAnyTemplateArgument, internal::Matcher, 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 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 isSameOrDerivedFrom( /// but not \c B. AST_MATCHER_P(CXXRecordDecl, hasMethod, internal::Matcher, 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, containsDeclaration, unsigned, N, /// record matches Foo, hasAnyConstructorInitializer matches foo_(1) AST_MATCHER_P(CXXConstructorDecl, hasAnyConstructorInitializer, internal::Matcher, 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, isImplicit) { /// 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, InnerMatcher) { TOOLING_COMPILE_ASSERT((llvm::is_base_of::value || @@ -2156,8 +2149,10 @@ AST_POLYMORPHIC_MATCHER_P(hasAnyArgument, internal::Matcher, 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, hasParameter, /// matching int y AST_MATCHER_P(FunctionDecl, hasAnyParameter, internal::Matcher, 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, internal::Matcher, /// matching '{}' AST_MATCHER_P(CompoundStmt, hasAnySubstatement, internal::Matcher, 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, hasObjectExpression, /// matches \code using X::b \endcode AST_MATCHER_P(UsingDecl, hasAnyUsingShadowDecl, internal::Matcher, 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, Stmt*, Other, 1) { /// "switch (1)", "switch (2)" and "switch (2)". AST_MATCHER_P(SwitchStmt, forEachSwitchCase, internal::Matcher, 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, forEachSwitchCase, internal::Matcher, 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; } diff --git a/include/clang/ASTMatchers/ASTMatchersInternal.h b/include/clang/ASTMatchers/ASTMatchersInternal.h index 6428ad85a7..b27ec57f7d 100644 --- a/include/clang/ASTMatchers/ASTMatchersInternal.h +++ b/include/clang/ASTMatchers/ASTMatchersInternal.h @@ -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(); } - /// \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 IDToNodeMap; IDToNodeMap NodeMap; }; -/// \brief A tree of bound nodes in match results. -/// -/// 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. +/// \brief Creates BoundNodesTree objects. /// -/// 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 RecursiveBindings); + /// \brief Add a binding from an id to a node. + template 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 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 - 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 RecursiveBindings; + SmallVector 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 makeMatcher(MatcherInterface *Implementation) { return Matcher(Implementation); } +/// \brief Finds the first node in a range that matches the given matcher. +template +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 +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 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: diff --git a/lib/ASTMatchers/ASTMatchFinder.cpp b/lib/ASTMatchers/ASTMatchFinder.cpp index 378453d851..d8f058848d 100644 --- a/lib/ASTMatchers/ASTMatchFinder.cpp +++ b/lib/ASTMatchers/ASTMatchFinder.cpp @@ -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 MatchCallback; // 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 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()) 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 InsertResult - = ResultCache.insert(std::make_pair(input, MemoizedMatchResult())); + if (ResultCache.size() > MaxMemoizationEntries) + ResultCache.clear(); + std::pair 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 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 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_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 > TypeAliases; // Maps (matcher, node) -> the match result for memoization. - typedef llvm::DenseMap MemoizationMap; + typedef std::map MemoizationMap; MemoizationMap ResultCache; }; @@ -616,8 +662,11 @@ bool MatchASTVisitor::classIsDerivedFrom(const CXXRecordDecl *Declaration, 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; } diff --git a/lib/ASTMatchers/ASTMatchersInternal.cpp b/lib/ASTMatchers/ASTMatchersInternal.cpp index 3144261612..757d20675b 100644 --- a/lib/ASTMatchers/ASTMatchersInternal.cpp +++ b/lib/ASTMatchers/ASTMatchersInternal.cpp @@ -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 RecursiveBindings) - : Bindings(Bindings), - RecursiveBindings(RecursiveBindings) {} - -void BoundNodesTree::copyTo(BoundNodesTreeBuilder* Builder) const { - Bindings.copyTo(Builder); - for (std::vector::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; } diff --git a/unittests/ASTMatchers/ASTMatchersTest.cpp b/unittests/ASTMatchers/ASTMatchersTest.cpp index 86b54acdeb..9f0d7afb95 100644 --- a/unittests/ASTMatchers/ASTMatchersTest.cpp +++ b/unittests/ASTMatchers/ASTMatchersTest.cpp @@ -3009,12 +3009,13 @@ TEST(ForEachDescendant, NestedForEachDescendant) { recordDecl(hasName("A"), anyOf(m, forEachDescendant(m))), new VerifyIdIsBoundTo("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("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("x", "C"))); } TEST(ForEachDescendant, BindsMultipleNodes) { @@ -3034,6 +3035,112 @@ TEST(ForEachDescendant, BindsRecursiveCombinations) { new VerifyIdIsBoundTo("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("if", 6))); +} + +TEST(Has, DoesNotDeleteBindings) { + EXPECT_TRUE(matchAndVerifyResultTrue( + "class X { int a; };", recordDecl(decl().bind("x"), has(fieldDecl())), + new VerifyIdIsBoundTo("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("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "class X {};", recordDecl(recordDecl().bind("x"), hasName("::X"), + anyOf(unless(anything()), anything())), + new VerifyIdIsBoundTo("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "template class X {}; X x;", + classTemplateSpecializationDecl( + decl().bind("x"), + hasAnyTemplateArgument(refersToType(asString("int")))), + new VerifyIdIsBoundTo("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "class X { void f(); void g(); };", + recordDecl(decl().bind("x"), hasMethod(hasName("g"))), + new VerifyIdIsBoundTo("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("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "void x(int, int) { x(0, 42); }", + callExpr(expr().bind("x"), hasAnyArgument(integerLiteral(equals(42)))), + new VerifyIdIsBoundTo("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "void x(int, int y) {}", + functionDecl(decl().bind("x"), hasAnyParameter(hasName("y"))), + new VerifyIdIsBoundTo("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "void x() { return; if (true) {} }", + functionDecl(decl().bind("x"), + has(compoundStmt(hasAnySubstatement(ifStmt())))), + new VerifyIdIsBoundTo("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("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "class A{}; class B{}; class C : B, A {};", + recordDecl(decl().bind("x"), isDerivedFrom("::A")), + new VerifyIdIsBoundTo("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("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "class A { class B { void f() {} }; };", + functionDecl(decl().bind("x"), hasAncestor(recordDecl(hasName("::A")))), + new VerifyIdIsBoundTo("x", 1))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "template struct A { struct B {" + " void f() { if(true) {} }" + "}; };" + "void t() { A::B b; b.f(); }", + ifStmt(stmt().bind("x"), hasAncestor(recordDecl(hasName("::A")))), + new VerifyIdIsBoundTo("x", 2))); + EXPECT_TRUE(matchAndVerifyResultTrue( + "class A {};", + recordDecl(hasName("::A"), decl().bind("x"), unless(hasName("fooble"))), + new VerifyIdIsBoundTo("x", 1))); +} + TEST(ForEachDescendant, BindsCorrectNodes) { EXPECT_TRUE(matchAndVerifyResultTrue( "class C { void f(); int i; };",