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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:42-45
+  if (Decl->getKind() != Decl::Kind::Namespace)
+    return false;
+
+  auto const *NS = reinterpret_cast<NamespaceDecl const *>(Decl);
----------------
Use proper casting, `isa<>()`, `dyn_cast<>`, so on.


================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:79
+    const MatchFinder::MatchResult &Result) {
+  NamespaceDecl const &ND = *Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
+  SourceManager const &Sources = *Result.SourceManager;
----------------
Here and everywhere - the consts are on the wrong side.


================
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);
----------------
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?


================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29-32
+  struct NamespaceContext {
+    std::string Name;
+    SourceLocation Begin, NameBegin, RBrace;
+  };
----------------
This looks like a pessimization.
I think you should just store `NamespaceDecl*`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136