From: Samuel Benzaquen <sbenza@google.com>
Date: Tue, 19 Apr 2016 15:52:56 +0000 (+0000)
Subject: [ASTMatchers] Do not try to memoize nodes we can't compare.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=729060f60b59e21c99925aca1f60c33a287699a8;p=clang

[ASTMatchers] Do not try to memoize nodes we can't compare.

Summary:
Prevent hasAncestor from comparing nodes that are not supported.
hasDescendant was fixed some time ago to avoid this problem.
I'm applying the same fix to hasAncestor: if any object in the Builder map is
not comparable, skip the cache.

Reviewers: alexfh

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D19231

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@266748 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/ASTMatchers/ASTMatchFinder.cpp b/lib/ASTMatchers/ASTMatchFinder.cpp
index e13985bca8..eed1a182b2 100644
--- a/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -616,6 +616,10 @@ private:
         ActiveASTContext->getTranslationUnitDecl())
       return false;
 
+    // For AST-nodes that don't have an identity, we can't memoize.
+    if (!Builder->isComparable())
+      return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
+
     MatchKey Key;
     Key.MatcherID = Matcher.getID();
     Key.Node = Node;
@@ -630,22 +634,34 @@ private:
     }
 
     MemoizedMatchResult Result;
-    Result.ResultOfMatch = false;
     Result.Nodes = *Builder;
+    Result.ResultOfMatch =
+        matchesAncestorOfRecursively(Node, Matcher, &Result.Nodes, MatchMode);
+
+    MemoizedMatchResult &CachedResult = ResultCache[Key];
+    CachedResult = std::move(Result);
+
+    *Builder = CachedResult.Nodes;
+    return CachedResult.ResultOfMatch;
+  }
 
+  bool matchesAncestorOfRecursively(const ast_type_traits::DynTypedNode &Node,
+                                    const DynTypedMatcher &Matcher,
+                                    BoundNodesTreeBuilder *Builder,
+                                    AncestorMatchMode MatchMode) {
     const auto &Parents = ActiveASTContext->getParents(Node);
     assert(!Parents.empty() && "Found node that is not in the parent map.");
     if (Parents.size() == 1) {
       // Only one parent - do recursive memoization.
       const ast_type_traits::DynTypedNode Parent = Parents[0];
-      if (Matcher.matches(Parent, this, &Result.Nodes)) {
-        Result.ResultOfMatch = true;
-      } else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
-        // Reset the results to not include the bound nodes from the failed
-        // match above.
-        Result.Nodes = *Builder;
-        Result.ResultOfMatch = memoizedMatchesAncestorOfRecursively(
-            Parent, Matcher, &Result.Nodes, MatchMode);
+      BoundNodesTreeBuilder BuilderCopy = *Builder;
+      if (Matcher.matches(Parent, this, &BuilderCopy)) {
+        *Builder = std::move(BuilderCopy);
+        return true;
+      }
+      if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+        return memoizedMatchesAncestorOfRecursively(Parent, Matcher, Builder,
+                                                    MatchMode);
         // Once we get back from the recursive call, the result will be the
         // same as the parent's result.
       }
@@ -655,10 +671,10 @@ private:
       std::deque<ast_type_traits::DynTypedNode> Queue(Parents.begin(),
                                                       Parents.end());
       while (!Queue.empty()) {
-        Result.Nodes = *Builder;
-        if (Matcher.matches(Queue.front(), this, &Result.Nodes)) {
-          Result.ResultOfMatch = true;
-          break;
+        BoundNodesTreeBuilder BuilderCopy = *Builder;
+        if (Matcher.matches(Queue.front(), this, &BuilderCopy)) {
+          *Builder = std::move(BuilderCopy);
+          return true;
         }
         if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
           for (const auto &Parent :
@@ -673,12 +689,7 @@ private:
         Queue.pop_front();
       }
     }
-
-    MemoizedMatchResult &CachedResult = ResultCache[Key];
-    CachedResult = std::move(Result);
-
-    *Builder = CachedResult.Nodes;
-    return CachedResult.ResultOfMatch;
+    return false;
   }
 
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
diff --git a/unittests/ASTMatchers/ASTMatchersTest.cpp b/unittests/ASTMatchers/ASTMatchersTest.cpp
index 7d5cba9086..7bb9550cd9 100644
--- a/unittests/ASTMatchers/ASTMatchersTest.cpp
+++ b/unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -708,6 +708,19 @@ TEST(DeclarationMatcher, HasDescendantMemoizationUsesRestrictKind) {
                       decl(anyOf(hasDescendant(RD), hasDescendant(VD)))));
 }
 
+TEST(DeclarationMatcher, HasAncestorMemoization) {
+  // This triggers an hasAncestor with a TemplateArgument in the bound nodes.
+  // That node can't be memoized so we have to check for it before trying to put
+  // it on the cache.
+  DeclarationMatcher CannotMemoize = classTemplateSpecializationDecl(
+      hasAnyTemplateArgument(templateArgument().bind("targ")),
+      forEach(fieldDecl(hasAncestor(forStmt()))));
+
+  EXPECT_TRUE(notMatches("template <typename T> struct S;"
+                         "template <> struct S<int>{ int i; int j; };",
+                         CannotMemoize));
+}
+
 TEST(DeclarationMatcher, HasAttr) {
   EXPECT_TRUE(matches("struct __attribute__((warn_unused)) X {};",
                       decl(hasAttr(clang::attr::WarnUnused))));