[cfe-commits] [PATCH] Implements hasAncestor.

[Daniel Jasper]

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:391
@@ -390,3 +390,3 @@
 ///
 /// This provides two entry methods for each base node type in the AST:
 /// - matchesChildOf:
----------------
.. three ..

Also, the "for each base node" is probably outdated, right?

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:392
@@ -391,3 +391,3 @@
 /// This provides two entry methods for each base node type in the AST:
 /// - matchesChildOf:
 ///   Matches a matcher on every child node of the given node. Returns true
----------------
Maybe \c matchesChildOf, also below...

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:461
@@ +460,3 @@
+                            llvm::is_base_of<Stmt, T>::value),
+                           only_Decl_or_Stmt_allowed_for_recursive_matching);
+    return matchesAncestorOf(ast_type_traits::DynTypedNode::create(Node),
----------------
Unlike descendants, all node types should have ancestors. Is it intentional to not allow this for QualTypes or possibly other types? 
- If there is a good reason for it: Comment.
- If it would be significant additional work: Command + FIXME
- Otherwise: Implement ;-)

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:71
@@ -68,1 +70,3 @@
 
+  bool isNull() const {
+    return Tag == NT_Null;
----------------
Maybe add:

  /// \brief \c isNull() returns whether this DynTypedNode contains a node.
  ///
  /// Will only return \c true for nodes constructed with the default
  /// constructor and \c false for nodes created by \c create(const T&). 

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:35
@@ +34,3 @@
+///
+/// FIXME: Currently only builds up the map using Stmt and Decl nodes.
+class ParentMapASTVisitor : public RecursiveASTVisitor<ParentMapASTVisitor> {
----------------
\c Stmt and \c Decl

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:36
@@ +35,3 @@
+/// FIXME: Currently only builds up the map using Stmt and Decl nodes.
+class ParentMapASTVisitor : public RecursiveASTVisitor<ParentMapASTVisitor> {
+public:
----------------
I think you need to add:

  bool shouldVisitTemplateInstantiations() const { return true; }
  bool shouldVisitImplicitCode() const { return true; }

So we are consistent with the other RecursiveASTVisitors for matching.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:370
@@ +369,3 @@
+      // We always need to run over the whole translation unit, as
+      // hasAncestor can escape any subtree.
+      Parents.reset(ParentMapASTVisitor::buildMap(
----------------
\c hasAncestor()

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:41
@@ +40,3 @@
+
+  /// Builds and returns the translation unit's parent map.
+  static ParentMap *buildMap(TranslationUnitDecl &TU) {
----------------
maybe:

  /// \brief Builds and returns the translation unit's parent map.
  ///
  /// The caller takes ownership of the returned \c ParentMap.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:366
@@ +365,3 @@
+    assert(Node.getMemoizationData() &&
+          "Ancestor matching is only allowed for nodes that have pointer "
+          "identity in the AST.");
----------------
I'd go with the more general message below in case we implement getMemoizationData() for other types.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:376
@@ +375,3 @@
+    ast_type_traits::DynTypedNode PreviousAncestor;
+    do {
+      PreviousAncestor = Ancestor;
----------------
a) PreviousAncestor is a confusing name. As we are walking back up the hierarchy, PreviousAncestor is usually a child of Ancestor ...
b) The abortion / assertion logic is weird. How about:

  ast_type_traits::DynTypedNode Ancestor = Node;
  while (Ancestor.get<TranslationUnitDecl>() != ActiveASTContext->getTranslationUnitDecl()) {
    Ancestor = Parents->lookup(Ancestor.getMemoizationData());
    assert(!Ancestor.isNull() && "Sneaked passed TUDecl");
    if (Matcher.matches(Ancestor, this, Builder))
      return true;
  }
  return false;
    

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:601
@@ -602,1 +600,3 @@
+// Returns from \c run whether \c BoundNodes contain a \c Decl bound to \c Id
+// that can be dynamically casted to \c T.
 // Optionally checks that the check succeeded a specific number of times.
----------------
... cast ...

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:600
@@ -599,3 +599,3 @@
 
-// Returns from Run whether 'bound_nodes' contain a Decl bound to 'Id', which
-// can be dynamically casted to T.
+// Returns from \c run whether \c BoundNodes contain a \c Decl bound to \c Id
+// that can be dynamically casted to \c T.
----------------
I don't fully understand that sentence structure, but that might be me...


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