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

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

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);
----------------
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.


https://reviews.llvm.org/D52136