wgml marked 2 inline comments as done.
wgml added inline comments.
================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+ if (childrenCount(ND.decls()) == 0) {
+ SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+ diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+ << FixItHint::CreateRemoval(Removal);
----------------
alexfh wrote:
> wgml wrote:
> > wgml wrote:
> > > lebedev.ri wrote:
> > > > 1. This is unrelated to the main check, so it should be in a separate
> > > > check.
> > > > 2. This is really fragile i think. What about preprocessor-driven
> > > > emptiness?
> > > 1. It seems to me that if I produce fixit to concat empty namespace (that
> > > is, execute the `else` branch), the fix is not applied as I intended -
> > > the namespace gets deleted.
> > > I do not fully understand this behavior and did not manage to find bug in
> > > the code. I also checked what will happen if I try replace the entire
> > > namespace with `namespace {}` and it is not getting inserted but simply
> > > deleted. On the other hand, I if replace with `namespace {;}`, I see the
> > > expected output. So, either it is really sneaky off-by-one somewhere, or
> > > clang does it's own thing.
> > > Truth be told, I added that `if` branch to handle such behavior
> > > explicitly instead of silently deleting namespace.
> > >
> > > 2. Can you provide an example when it will matter?
> > I dropped the "support" for removing. However, empty namespaces are still
> > being removed implicitly, by a fixit applying tool, I believe.
> >
> > I added entries to the test to make sure preprocessor stuff is never
> > removed.
> Removal of empty namespaces is intentional. Clang-tidy calls
> clang::format::cleanupAroundReplacements when applying fixes, which, in
> particular, removes empty namespaces. See code around
> clang/lib/Format/Format.cpp:1309. The motivation is that when different
> checks remove different parts of code inside a namespace and this results in
> an empty namespace, it's better to remove it. Other cleanups are, for
> example, removal of commas and the colon in constructor initializer lists.
That is good to know! Everything looks good to me then.
https://reviews.llvm.org/D52136
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
