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