[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

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