Page MenuHomePhabricator

[AST] Extract ASTDumpTraverser class from ASTDumper
ClosedPublic

Authored by steveire on Jan 30 2019, 1:49 PM.

Details

Summary

This new traverser class allows clients to re-use the traversal logic
which was previously part of ASTDumper. This means that alternative
visit logic may be implemented, such as

  • Dump to alternative data formats such as JSON
  • Implement AST Matcher parent/child visitation matching AST dumps

Diff Detail

Repository
rL LLVM

Event Timeline

steveire created this revision.Jan 30 2019, 1:49 PM
steveire updated this revision to Diff 184363.Jan 30 2019, 1:51 PM

Fix comment

steveire marked an inline comment as done.Jan 30 2019, 1:57 PM
steveire added inline comments.
lib/AST/ASTDumper.cpp
63 ↗(On Diff #184363)

These visitors are left behind here because how their specializations are handled is quite bespoke in AST-dumping terms.

The way specializations are dumped could probably be cleaned up a bit to make it possible to move these methods too, but it is not clear what would be acceptable there.

aaron.ballman added inline comments.Jan 31 2019, 12:17 PM
include/clang/AST/ASTDumpTraverser.h
31 ↗(On Diff #184363)

Missing full stop at the end of the sentence.

38 ↗(On Diff #184363)

Format this with the usual LLVM style.

Also, I'd remove a bit of the vertical whitespace between the Visit() functions.

83 ↗(On Diff #184363)

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)?

91–92 ↗(On Diff #184363)

I wonder how often we will spot calls to Visit() that were meant to go to getNodeVisitor().Visit()? Probably not too many because the output would be wrong except if you got unlucky, but I do wonder if the naming similarities will cause confusion as people do new development.

Not certain there's anything to be changed here, but wanted to raise the point in case it sparked discussion.

lib/AST/ASTDumper.cpp
63 ↗(On Diff #184363)

That seems reasonable to me; we can address them in a later patch.

steveire marked an inline comment as done.Jan 31 2019, 2:40 PM
steveire added inline comments.
include/clang/AST/ASTDumpTraverser.h
83 ↗(On Diff #184363)

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);
  });
}
aaron.ballman accepted this revision.Feb 1 2019, 5:44 AM

LGTM with a renaming request.

include/clang/AST/ASTDumpTraverser.h
83 ↗(On Diff #184363)

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!

This revision is now accepted and ready to land.Feb 1 2019, 5:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 5:44 AM
steveire marked an inline comment as done.Feb 1 2019, 11:33 AM
steveire added inline comments.
include/clang/AST/ASTDumpTraverser.h
1 ↗(On Diff #184616)

I think I'll rename this to ASTNodeTraverser. Any objections?

aaron.ballman added inline comments.Feb 1 2019, 11:38 AM
include/clang/AST/ASTDumpTraverser.h
1 ↗(On Diff #184616)

Nope, I think that's a good idea.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2019, 6:08 AM