[PATCH] D55188: Extract TextChildDumper class

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump
----------------
steveire wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain this namespace is useful, especially when it gets imported at TU scope in ASTDumper.cpp.
> > > > 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.
> > 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.
> There is no need for such churn as adding the namespace later.
> 
> The `using` I added is beside two other `using`s in a cpp file. There is nothing 'global' about it.
> 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188