[PATCH] Completely revamp node binding for AST matchers.

[Edwin Vane]

  A few style nits.

  Most of my comments are questions about how things work and why you did things as you did. You may interpret these as requests for additional code comments if you see fit.


================
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 {
----------------
What other nodes? Non-DynTypedNodes?

================
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);
----------------
Should single-line for loops have no braces as with if statements?

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:870
@@ -892,1 +869,3 @@
+    BoundNodesTreeBuilder Discard(*Builder);
+    return !InnerMatcher.matches(Node, Finder, &Discard);
   }
----------------
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?

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:896
@@ +895,3 @@
+        !InnerMatcher2.matches(Node, Finder, Builder)) {
+      return false;
+    }
----------------
Single line if with braces.

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

================
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")));
----------------
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?

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3040
@@ +3039,3 @@
+      compoundStmt(forEachDescendant(ifStmt().bind("if")),
+                   forEachDescendant(whileStmt().bind("while"))),
+      new VerifyIdIsBoundTo<IfStmt>("if", 6)));
----------------
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?

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3051
@@ +3050,3 @@
+
+TEST(LoopingMatchers, DoNotOverwritePreviousMatchResultOnFailure) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
----------------
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?


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