[PATCH] Implements memoization for ancestor matching.

[Manuel Klimek]

    - Adds a test that shows that we really want BFS and implements BFS.

Hi djasper,

http://llvm-reviews.chandlerc.com/D510

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D510?vs=1222&id=1280#toc

Files:
  lib/ASTMatchers/ASTMatchFinder.cpp
  unittests/ASTMatchers/ASTMatchersTest.cpp

Index: lib/ASTMatchers/ASTMatchFinder.cpp
===================================================================
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include <deque>
 #include <set>
 
 namespace clang {
@@ -388,7 +389,8 @@
                                  const DynTypedMatcher &Matcher,
                                  BoundNodesTreeBuilder *Builder,
                                  AncestorMatchMode MatchMode) {
-    return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
+    return memoizedMatchesAncestorOfRecursively(Node, Matcher, Builder,
+                                                MatchMode);
   }
 
   // Matches all registered matchers on the given node and calls the
@@ -421,7 +423,21 @@
   bool shouldUseDataRecursionFor(clang::Stmt *S) const { return false; }
 
 private:
-  bool matchesAncestorOfRecursively(
+  // Returns whether an ancestor of \p Node matches \p Matcher.
+  //
+  // The oder of matching (which is visible through which node is bound in case
+  // there are multiple matches) is breadth first search.
+  //
+  // To allow memoization in the very common case of having deeply nested
+  // expressions inside a template function, we first walk up the AST, memoizing
+  // the result of the match along the way, as long as there is only a single
+  // parent.
+  //
+  // Once there are multiple parents, the result of a hasAncestor(...) matcher
+  // in terms of which node was bound is not necessarily the same as the result
+  // of breadth first search on a child node, thus we do not memoize ancestors
+  // below split points.
+  bool memoizedMatchesAncestorOfRecursively(
       const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher &Matcher,
       BoundNodesTreeBuilder *Builder, AncestorMatchMode MatchMode) {
     if (Node.get<TranslationUnitDecl>() ==
@@ -435,24 +451,48 @@
       assert(false && "Found node that is not in the parent map.");
       return false;
     }
-    for (ASTContext::ParentVector::const_iterator AncestorI = Parents.begin(),
-                                                  AncestorE = Parents.end();
-         AncestorI != AncestorE; ++AncestorI) {
-      if (Matcher.matches(*AncestorI, this, Builder))
-        return true;
-    }
-    if (MatchMode == ASTMatchFinder::AMM_ParentOnly)
-      return false;
-    for (ASTContext::ParentVector::const_iterator AncestorI = Parents.begin(),
-                                                  AncestorE = Parents.end();
-         AncestorI != AncestorE; ++AncestorI) {
-      if (matchesAncestorOfRecursively(*AncestorI, Matcher, Builder, MatchMode))
-        return true;
+    const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
+    MemoizationMap::iterator I = ResultCache.find(input);
+    if (I == ResultCache.end()) {
+      BoundNodesTreeBuilder AncestorBoundNodesBuilder;
+      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)) {
+          Matches = true;
+        } else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+          Matches = memoizedMatchesAncestorOfRecursively(
+              Parent, Matcher, &AncestorBoundNodesBuilder, MatchMode);
+        }
+      } else {
+        // Multiple parents - BFS over the rest of the nodes.
+        std::deque<ast_type_traits::DynTypedNode> Queue(Parents.begin(),
+                                                        Parents.end());
+        while (!Queue.empty()) {
+          if (Matcher.matches(Queue.front(), this,
+                              &AncestorBoundNodesBuilder)) {
+            Matches = true;
+            break;
+          }
+          if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+            ASTContext::ParentVector Ancestors =
+                ActiveASTContext->getParents(Queue.front());
+            Queue.insert(Queue.end(), Ancestors.begin(), Ancestors.end());
+          }
+          Queue.pop_front();
+        }
+      }
+
+      I = ResultCache.insert(std::make_pair(input, MemoizedMatchResult()))
+          .first;
+      I->second.Nodes = AncestorBoundNodesBuilder.build();
+      I->second.ResultOfMatch = Matches;
     }
-    return false;
+    I->second.Nodes.copyTo(Builder);
+    return I->second.ResultOfMatch;
   }
 
-
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
   // the aggregated bound nodes for each match.
   class MatchVisitor : public BoundNodesTree::Visitor {
Index: unittests/ASTMatchers/ASTMatchersTest.cpp
===================================================================
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -632,8 +632,10 @@
   // Create an object that checks that a node of type \c T was bound to \c Id.
   // Checks that there was exactly one match with the name \c ExpectedName.
   // Note that \c T must be a NamedDecl for this to work.
-  VerifyIdIsBoundTo(llvm::StringRef Id, llvm::StringRef ExpectedName)
-    : Id(Id), ExpectedCount(1), Count(0), ExpectedName(ExpectedName) {}
+  VerifyIdIsBoundTo(llvm::StringRef Id, llvm::StringRef ExpectedName,
+                    int ExpectedCount = 1)
+      : Id(Id), ExpectedCount(ExpectedCount), Count(0),
+        ExpectedName(ExpectedName) {}
 
   ~VerifyIdIsBoundTo() {
     if (ExpectedCount != -1)
@@ -3192,6 +3194,20 @@
       new VerifyIdIsBoundTo<CXXRecordDecl>("d", "E")));
 }
 
+TEST(HasAncestor, MatchesClosestAncestor) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "template <typename T> struct C {"
+      "  void f(int) {"
+      "    struct I { void g(T) { int x; } } i; i.g(42);"
+      "  }"
+      "};"
+      "template struct C<int>;",
+      varDecl(hasName("x"),
+              hasAncestor(functionDecl(hasParameter(
+                  0, varDecl(hasType(asString("int"))))).bind("f"))).bind("v"),
+      new VerifyIdIsBoundTo<FunctionDecl>("f", "g", 2)));
+}
+
 TEST(HasAncestor, MatchesInTemplateInstantiations) {
   EXPECT_TRUE(matches(
       "template <typename T> struct A { struct B { struct C { T t; }; }; }; "
@@ -3254,6 +3270,10 @@
                              hasParent(recordDecl(isTemplateInstantiation())))),
                          hasParent(functionDecl(hasParent(recordDecl(
                              unless(isTemplateInstantiation())))))))))));
+  EXPECT_TRUE(
+      notMatches("template <typename T> struct C { static void f() {} };"
+                 "void t() { C<int>::f(); }",
+                 compoundStmt(hasParent(recordDecl()))));
 }
 
 TEST(TypeMatching, MatchesTypes) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D510.2.patch
Type: text/x-patch
Size: 6857 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130313/192e260b/attachment.bin>