[cfe-commits] [PATCH] Implements hasAncestor.

[Manuel Klimek]

================
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:
----------------
Daniel Jasper wrote:
> .. three ..
> 
> Also, the "for each base node" is probably outdated, right?
Done.

================
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
----------------
Daniel Jasper wrote:
> Maybe \c matchesChildOf, also below...
Done.

================
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),
----------------
Daniel Jasper wrote:
> 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 ;-)
Like with the other methods here, there are many more nodes for which we want them implemented than are currently implemented.

You're right that for hasAncestor probably pretty much all nodes make sense. Added a class-level comment that outlines the differences.

================
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> {
----------------
Daniel Jasper wrote:
> \c Stmt and \c Decl
Done.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:41
@@ +40,3 @@
+
+  /// Builds and returns the translation unit's parent map.
+  static ParentMap *buildMap(TranslationUnitDecl &TU) {
----------------
Daniel Jasper wrote:
> maybe:
> 
>   /// \brief Builds and returns the translation unit's parent map.
>   ///
>   /// The caller takes ownership of the returned \c ParentMap.
Done.

================
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.");
----------------
Daniel Jasper wrote:
> I'd go with the more general message below in case we implement getMemoizationData() for other types.
Agreed. Done.

================
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(
----------------
Daniel Jasper wrote:
> \c hasAncestor()
Done.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:376
@@ +375,3 @@
+    ast_type_traits::DynTypedNode PreviousAncestor;
+    do {
+      PreviousAncestor = Ancestor;
----------------
Daniel Jasper wrote:
> 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;
>     
Done, and, as discussed offline, removed the isNull() stuff entirely.

================
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.
----------------
Daniel Jasper wrote:
> I don't fully understand that sentence structure, but that might be me...
Done.

================
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.
----------------
Daniel Jasper wrote:
> ... cast ...
Done.

================
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:
----------------
Daniel Jasper wrote:
> 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.
Done.


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