This is an archive of the discontinued LLVM Phabricator instance.

Extract TextChildDumper class
ClosedPublic

Authored by steveire on Dec 3 2018, 1:44 AM.

Diff Detail

Event Timeline

steveire created this revision.Dec 3 2018, 1:44 AM
aaron.ballman added inline comments.Dec 3 2018, 5:25 AM
include/clang/AST/ASTDumperUtils.h
22

I'm not certain this namespace is useful, especially when it gets imported at TU scope in ASTDumper.cpp.

96

I'm not sold on the name for this class. It's a bit too generic to understand what it does. How about ASTDumpLayoutFormatter (and ASTDumpNodeFormatter for the node dumper)?

97–110

I don't think these members should be public.

steveire marked an inline comment as done.Dec 3 2018, 6:08 AM
steveire added inline comments.
include/clang/AST/ASTDumperUtils.h
22

it gets imported at TU scope in ASTDumper.cpp

Today that's the only place it is used. In the future that won't be true.

aaron.ballman added inline comments.Dec 3 2018, 6:16 AM
include/clang/AST/ASTDumperUtils.h
22

Then we can add the namespace in the future when we find a situation where we're worried about collisions? As it stands, this only provides the illusion of safety. If you really want to keep it, please pull the global using declaration.

steveire marked 2 inline comments as done.Dec 3 2018, 8:46 AM
steveire added inline comments.
include/clang/AST/ASTDumperUtils.h
22

There is no need for such churn as adding the namespace later.

The using I added is beside two other usings in a cpp file. There is nothing 'global' about it.

96

I'll rename the class to ASTTextTreeStructure.

steveire marked an inline comment as done.Dec 3 2018, 8:52 AM
steveire added inline comments.
include/clang/AST/ASTDumperUtils.h
96

Or rather, TextTreeStructure.

aaron.ballman added inline comments.Dec 3 2018, 12:08 PM
include/clang/AST/ASTDumperUtils.h
22

The using I added is beside two other usings in a cpp file. There is nothing 'global' about it.

It applies to the entire TU, so it's global as far as this compilation unit is concerned. The existing "clang" and "llvm" using declarations are a pervasive thing we do but is not the general rule for how we use namespaces. See https://llvm.org/docs/CodingStandards.html#do-not-use-using-namespace-std for more details.

I think the idea of an ast dumping namespace is worth exploring, but is orthogonal to this NFC patch. If you'd like to propose putting all AST dumping functionality into its own namespace, then please propose it as a separate patch so that we don't bog this one down with discussion that's not really germane to the rest of your patch. Questions I would love to see answered in that review are: why is a namespace needed, what should go into it (for instance, does ast printing go there, or just ast dumping, or all ast output utilities, etc), and whether it's planned to be used outside of its implementation file(s).

Alternatively, if you think this is germane to this patch, we can have that discussion here.

steveire updated this revision to Diff 176466.Dec 3 2018, 1:18 PM

Rename class.

steveire updated this revision to Diff 176467.Dec 3 2018, 1:26 PM

Remove namespace

aaron.ballman accepted this revision.Dec 4 2018, 2:07 PM

LGTM aside from a commenting nit.

include/clang/AST/ASTDumperUtils.h
113

I really like the rename, but can you update this comment to not say "dump" as that implies to me that the writing happens immediately. It might also be a kindness to add the expected function signature of Fn to the comments.

This revision is now accepted and ready to land.Dec 4 2018, 2:07 PM
This revision was automatically updated to reflect the committed changes.