From: Daniel Jasper <djasper@google.com>
Date: Fri, 16 Nov 2012 18:39:22 +0000 (+0000)
Subject: Fix partial-match-bind-behavior with forEachDescendant() matchers.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5f684e90c2f44eda979573a01c2ed063d9adc7a8;p=clang

Fix partial-match-bind-behavior with forEachDescendant() matchers.

The problem is that a partial match of an (explicit or implicit) allOf matcher
binds results, i.e.

recordDecl(decl().bind("x"), hasName("A"))

can very well bind a record that is not named "A". With this fix, the common
cases of stumbling over this bug are fixed by the BoundNodesMap overwriting the
results of a partial match. An error can still be created with a weird
combination of anyOf and allOf (see inactive test). We need to decide whether
this is worth fixing, as the fix will have performance impact.

Review: http://llvm-reviews.chandlerc.com/D124

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

diff --git a/lib/ASTMatchers/ASTMatchersInternal.cpp b/lib/ASTMatchers/ASTMatchersInternal.cpp
index 408195d369..f1a9ff2e09 100644
--- a/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -27,8 +27,11 @@ void BoundNodesMap::copyTo(BoundNodesTreeBuilder *Builder) const {
 }
 
 void BoundNodesMap::copyTo(BoundNodesMap *Other) const {
-  copy(NodeMap.begin(), NodeMap.end(),
-       inserter(Other->NodeMap, Other->NodeMap.begin()));
+  for (IDToNodeMap::const_iterator I = NodeMap.begin(),
+                                   E = NodeMap.end();
+       I != E; ++I) {
+    Other->NodeMap[I->first] = I->second;
+  }
 }
 
 BoundNodesTree::BoundNodesTree() {}
diff --git a/unittests/ASTMatchers/ASTMatchersTest.cpp b/unittests/ASTMatchers/ASTMatchersTest.cpp
index 6d8c00058b..3eddccc652 100644
--- a/unittests/ASTMatchers/ASTMatchersTest.cpp
+++ b/unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -2777,6 +2777,22 @@ TEST(ForEachDescendant, BindsOneNode) {
       new VerifyIdIsBoundTo<FieldDecl>("x", 1)));
 }
 
+TEST(ForEachDescendant, NestedForEachDescendant) {
+  DeclarationMatcher m = recordDecl(
+      isDefinition(), decl().bind("x"), hasName("C"));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+    "class A { class B { class C {}; }; };",
+    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")));
+}
+
 TEST(ForEachDescendant, BindsMultipleNodes) {
   EXPECT_TRUE(matchAndVerifyResultTrue(
       "class C { class D { int x; int y; }; "