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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
