[PATCH] D57472: [AST] Extract ASTDumpTraverser class from ASTDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Herald added a project: clang.

LGTM with a renaming request.



================
Comment at: include/clang/AST/ASTDumpTraverser.h:83
+
+  NodeVisitorType &getNodeVisitor() { return getDerived().doGetNodeVisitor(); }
+  Derived &getDerived() { return *static_cast<Derived *>(this); }
----------------
steveire wrote:
> aaron.ballman wrote:
> > Given that `ASTDumpTraverser` is itself a visitor of nodes, I wonder if a better name for this would be `getNodeDumper()` or something (and similarly renaming the template parameter)?
> I'm not opposed to alternative names, but I do like the current name. We `visit` each node before traversing.
> 
> `NodeVisitor` names make sense to me because 'dumping' isn't necessarily what the delegate class does. It is more consistent with what 'Visitor' usually means.
> 
> Perhaps a type name of `NodeDelegateType` and an accessor `NodeDelegateType &getNodeDelegate()` would also make sense. Then we would have eg:
> 
> ```
>   void Visit(const Attr *A) {
>     getNodeDelegate().AddChild([=] {
>       getNodeDelegate().Visit(A);
>       ConstAttrVisitor<Derived>::Visit(A);
>     });
>   }
> ```
> 
> NodeVisitor names make sense to me because 'dumping' isn't necessarily what the delegate class does. It is more consistent with what 'Visitor' usually means.

Very true.

> Perhaps a type name of NodeDelegateType and an accessor NodeDelegateType &getNodeDelegate() would also make sense. 

I think this is an improvement over `getNodeVisitor()`  -- good suggestion!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57472/new/

https://reviews.llvm.org/D57472