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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
