This is an archive of the discontinued LLVM Phabricator instance.

NFC: Move dump of type nodes to NodeDumper
ClosedPublic

Authored by steveire on Jan 12 2019, 9:15 AM.

Diff Detail

Event Timeline

steveire created this revision.Jan 12 2019, 9:15 AM

Mostly looks good (a few minor nits), but I did have a design question about whether we should prefer calling a Visit method for type hierarchies instead of reimplementing the functionality.

lib/AST/ASTDumper.cpp
145

Why this approach instead of deferring to VisitArrayType() as before? I prefer calling the Visit function rather than reimplementing the functionality because we may decide to later improve the base class printing and expect subclasses to automatically pick that up. WDYT?

164

Why this approach as opposed to deferring to VisitFunctionType() as before?

lib/AST/TextNodeDumper.cpp
14–16

Remove newline

960

It'd be nice to remove this use of auto.

971

It'd be nice to remove this use of auto.

1047

It'd be nice to remove this use of auto.

steveire marked an inline comment as done.Jan 14 2019, 6:31 AM
steveire added inline comments.
lib/AST/ASTDumper.cpp
145

I think it's more clear to read what each visit method does, but I don't feel strongly about it. I can change this if you do.

aaron.ballman added inline comments.Jan 14 2019, 6:43 AM
lib/AST/ASTDumper.cpp
145

Given that there's an established type hierarchy, I prefer calling the Visit() function for the superclass rather than reimplementing. The extra indirection wasn't too annoying with the old code, so it's unlikely to be too problematic with the new code (and if it is, then we'll have reason to change).

aaron.ballman accepted this revision.Jan 14 2019, 2:27 PM

LGTM aside from auto nits.

This revision is now accepted and ready to land.Jan 14 2019, 2:27 PM
This revision was automatically updated to reflect the committed changes.