[PATCH] Completely revamp node binding for AST matchers.

[Manuel Klimek]

================
Comment at: include/clang/AST/ASTTypeTraits.h:74
@@ +73,3 @@
+  /// Supports comparison of nodes that support memoization.
+  /// FIXME: Implement comparsion for other nodes.
+  bool operator<(const DynTypedNode &Other) const {
----------------
Edwin Vane wrote:
> What other nodes? Non-DynTypedNodes?
Added clarifying comment.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:126
@@ +125,3 @@
+      Bindings.push_back(BoundNodesMap());
+    for (unsigned i = 0, e = Bindings.size(); i != e; ++i) {
+      Bindings[i].addNode(Id, Node);
----------------
Edwin Vane wrote:
> Should single-line for loops have no braces as with if statements?
Fixed.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:870
@@ -892,1 +869,3 @@
+    BoundNodesTreeBuilder Discard(*Builder);
+    return !InnerMatcher.matches(Node, Finder, &Discard);
   }
----------------
Edwin Vane wrote:
> I can't think of a useful case of binding inside an unless() matcher but I suppose this impl means that any bindings would be lost anyway right?
Yep, exactly.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:896
@@ +895,3 @@
+        !InnerMatcher2.matches(Node, Finder, Builder)) {
+      return false;
+    }
----------------
Edwin Vane wrote:
> Single line if with braces.
Just noted that this was a leftover from where I had debug info in there :) Now there's only a return ...

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:373
@@ -348,2 +372,3 @@
+    if (!Node.getMemoizationData()) {
       return matchesRecursively(Node, Matcher, Builder, MaxDepth, Traversal,
                                 Bind);
----------------
Edwin Vane wrote:
> Single statement `if` with braces
Fixed.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3014
@@ +3013,3 @@
+    "class A { class B { class C {}; }; };",
+    recordDecl(hasName("A"), allOf(hasDescendant(m), anyOf(m, anything()))),
+    new VerifyIdIsBoundTo<Decl>("x", "C")));
----------------
Edwin Vane wrote:
> What's the goal of putting 'm' in the anyOf() here? It will always fail right? Is there something special about the binding mechanics you're testing?
Yea, that's non-obvious. Added clarifying comment.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3040
@@ +3039,3 @@
+      compoundStmt(forEachDescendant(ifStmt().bind("if")),
+                   forEachDescendant(whileStmt().bind("while"))),
+      new VerifyIdIsBoundTo<IfStmt>("if", 6)));
----------------
Edwin Vane wrote:
> I understand this is how you want this sort of matcher to behave but I don't understand why two forEachDescendants() should produce a cross product. Why would you want that?
Because:
a) from talking to different people it is much more aligned with what they expect
b) it opens a whole new level of power, without adding conceptual complexity (I think it actually conceptually simplifies things)
c) it gets rid of annoyances in the semantics we had before
d) it'll lead to the equalsBoundNode matcher being 1) trivial to implement and 2) intuitively understandable

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3051
@@ +3050,3 @@
+
+TEST(LoopingMatchers, DoNotOverwritePreviousMatchResultOnFailure) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
----------------
Edwin Vane wrote:
> There are a lot of tests here all following the same pattern: do a bind and have a sub-matcher fail that doesn't cause the whole expression to fail. Are these tests meant to be an exhaustive list of matchers that can fail? Why did you choose these particular tests?
All changes made in this CL that were not "simple refactorings" have been test driven.

That is: I write a test, see it fail, then implement enough to make it work.


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