[cfe-commits] [PATCH] Implements hasAncestor.

[Manuel Klimek]

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:460
@@ -451,1 +459,3 @@
 
+  // 
+  template <typename T>
----------------
Daniel Jasper wrote:
> Empty comment?
> 
> Maybe add
> 
> // FIXME: Implement support for BindKind in matchesAncestorOf.
Done.

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:834
@@ +833,3 @@
+  TOOLING_COMPILE_ASSERT(IsBaseType<AncestorT>::value,
+                         has_descendant_only_accepts_base_type_matcher);
+public:
----------------
Michael Diamond wrote:
> Copy/paste typo: has_ancestor, not has_descendant.
Done.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:33
@@ +32,3 @@
+/// \brief A \c RecursiveASTVisitor that builds a map from nodes to their
+/// parents.
+///
----------------
Daniel Jasper wrote:
> Should we add something like ".. to their parents as defined by the RecursiveASTVisitor"? I think there might not be another explicit definition of the node hierarchy and the behavior differs e.g. from Decl::getDeclContext().
Done.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:2798
@@ +2797,3 @@
+      "struct X {}; struct A { A() {} X x; };",
+      constructorDecl(
+          hasAnyConstructorInitializer(withInitializer(expr(
----------------
Daniel Jasper wrote:
> I had to think about this for a bit because actually the hasAncestor does not match implicit code. The important point is that the ParentMap contains implicit code. Maybe rename to "TEST(HasAncestor, ParentMapIncludesImplicitCode)"?
Hm, I usually read matchers in matching the type that they are called on. Thus, hasAncestor here matches on the initializer expression, which is implicit (or not?).

I'd rather not include implementation details in the test names.


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