[clang-tidy] avoid colon in namespace cause false positve

[HerrCai0907]

Summary

Refactor the Namespaces with NamespaceDecl[][].
First level stores non nested namepsace.
Second level stores nested namespace.

[PiotrZSL]

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp

175

debug log...

[PiotrZSL]

Except above llvm:errs, looks good to me.

[HerrCai0907]

remove debug log

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_CONCATNESTEDNAMESPACESCHECK_H
 
 #include "../ClangTidyCheck.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 
 namespace clang::tidy::modernize {
 
+using NamespaceName = llvm::SmallString<40>;
+
+class NS : public llvm::SmallVector<const NamespaceDecl *, 6> {
+public:
+  std::optional<SourceRange>
+  getCleanedNamespaceFrontRange(const SourceManager &SM,
+                                const LangOptions &LangOpts) const;
+  SourceRange getReplacedNamespaceFrontRange() const;
+  SourceRange getNamespaceBackRange(const SourceManager &SM,
+                                    const LangOptions &LangOpts) const;
+  SourceRange getDefaultNamespaceBackRange() const;
+  NamespaceName getName() const;
+};
+
 class ConcatNestedNamespacesCheck : public ClangTidyCheck {
 public:
   ConcatNestedNamespacesCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus17;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
-  using NamespaceContextVec = llvm::SmallVector<const NamespaceDecl *, 6>;
-  using NamespaceString = llvm::SmallString<40>;
+  using NamespaceContextVec = llvm::SmallVector<NS, 6>;
 
   void reportDiagnostic(const SourceManager &Sources,
                         const LangOptions &LangOpts);
-  NamespaceString concatNamespaces();
   NamespaceContextVec Namespaces;
 };
 } // namespace clang::tidy::modernize
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_CONCATNESTEDNAMESPACESCHECK_H

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp

 //===--- ConcatNestedNamespacesCheck.cpp - clang-tidy----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConcatNestedNamespacesCheck.h"
 #include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceLocation.h"
-#include "llvm/ADT/STLExtras.h"
 #include <algorithm>
 #include <optional>
 
 namespace clang::tidy::modernize {
 
 static bool locationsInSameFile(const SourceManager &Sources,
                                 SourceLocation Loc1, SourceLocation Loc2) {
   return Loc1.isFileID() && Loc2.isFileID() &&
static bool singleNamedNamespaceChild(const NamespaceDecl &ND) {
   NamespaceDecl::decl_range Decls = ND.decls();
   if (std::distance(Decls.begin(), Decls.end()) != 1)
     return false;
 
   const auto *ChildNamespace = dyn_cast<const NamespaceDecl>(*Decls.begin());
   return ChildNamespace && !unsupportedNamespace(*ChildNamespace);
 }
 
-static bool alreadyConcatenated(std::size_t NumCandidates,
-                                const SourceRange &ReplacementRange,
-                                const SourceManager &Sources,
-                                const LangOptions &LangOpts) {
-  // FIXME: This logic breaks when there is a comment with ':'s in the middle.
-  return getRawStringRef(ReplacementRange, Sources, LangOpts).count(':') ==
-         (NumCandidates - 1) * 2;
+template <class R, class F>
+static void concatNamespace(NamespaceName &ConcatNameSpace, R &&Range,
+                            F &&Stringify) {
+  for (auto const &V : Range) {
+    ConcatNameSpace.append(Stringify(V));
+    if (V != Range.back())
+      ConcatNameSpace.append("::");
+  }
 }
 
-static std::optional<SourceRange>
-getCleanedNamespaceFrontRange(const NamespaceDecl *ND, const SourceManager &SM,
-                              const LangOptions &LangOpts) {
+std::optional<SourceRange>
+NS::getCleanedNamespaceFrontRange(const SourceManager &SM,
+                                  const LangOptions &LangOpts) const {
   // Front from namespace tp '{'
   std::optional<Token> Tok =
       ::clang::tidy::utils::lexer::findNextTokenSkippingComments(
-          ND->getLocation(), SM, LangOpts);
+          back()->getLocation(), SM, LangOpts);
   if (!Tok)
     return std::nullopt;
   while (Tok->getKind() != tok::TokenKind::l_brace) {
     Tok = utils::lexer::findNextTokenSkippingComments(Tok->getEndLoc(), SM,
                                                       LangOpts);
     if (!Tok)
       return std::nullopt;
   }
-  return SourceRange{ND->getBeginLoc(), Tok->getEndLoc()};
+  return SourceRange{front()->getBeginLoc(), Tok->getEndLoc()};
+}
+SourceRange NS::getReplacedNamespaceFrontRange() const {
+  return SourceRange{front()->getBeginLoc(), back()->getLocation()};
 }
 
-static SourceRange getCleanedNamespaceBackRange(const NamespaceDecl *ND,
-                                                const SourceManager &SM,
-                                                const LangOptions &LangOpts) {
+SourceRange NS::getDefaultNamespaceBackRange() const {
+  return SourceRange{front()->getRBraceLoc(), front()->getRBraceLoc()};
+}
+SourceRange NS::getNamespaceBackRange(const SourceManager &SM,
+                                      const LangOptions &LangOpts) const {
   // Back from '}' to conditional '// namespace xxx'
-  const SourceRange DefaultSourceRange =
-      SourceRange{ND->getRBraceLoc(), ND->getRBraceLoc()};
-  SourceLocation Loc = ND->getRBraceLoc();
+  SourceLocation Loc = front()->getRBraceLoc();
   std::optional<Token> Tok =
       utils::lexer::findNextTokenIncludingComments(Loc, SM, LangOpts);
   if (!Tok)
-    return DefaultSourceRange;
+    return getDefaultNamespaceBackRange();
   if (Tok->getKind() != tok::TokenKind::comment)
-    return DefaultSourceRange;
+    return getDefaultNamespaceBackRange();
   SourceRange TokRange = SourceRange{Tok->getLocation(), Tok->getEndLoc()};
   StringRef TokText = getRawStringRef(TokRange, SM, LangOpts);
-  std::string CloseComment = "namespace " + ND->getNameAsString();
+  std::string CloseComment = ("namespace " + getName()).str();
   // current fix hint in readability/NamespaceCommentCheck.cpp use single line
   // comment
   if (TokText != "// " + CloseComment && TokText != "//" + CloseComment)
-    return DefaultSourceRange;
-  return SourceRange{ND->getRBraceLoc(), Tok->getEndLoc()};
+    return getDefaultNamespaceBackRange();
+  return SourceRange{front()->getRBraceLoc(), Tok->getEndLoc()};
 }
 
-ConcatNestedNamespacesCheck::NamespaceString
-ConcatNestedNamespacesCheck::concatNamespaces() {
-  NamespaceString Result("namespace ");
-  Result.append(Namespaces.front()->getName());
-
-  std::for_each(std::next(Namespaces.begin()), Namespaces.end(),
-                [&Result](const NamespaceDecl *ND) {
-                  Result.append("::");
-                  Result.append(ND->getName());
-                });
-
-  return Result;
+NamespaceName NS::getName() const {
+  NamespaceName Name{};
+  concatNamespace(Name, *this,
+                  [](const NamespaceDecl *ND) { return ND->getName(); });
+  return Name;
 }
 
 void ConcatNestedNamespacesCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(ast_matchers::namespaceDecl().bind("namespace"), this);
 }
 
 void ConcatNestedNamespacesCheck::reportDiagnostic(
     const SourceManager &SM, const LangOptions &LangOpts) {
   DiagnosticBuilder DB =
-      diag(Namespaces.front()->getBeginLoc(),
+      diag(Namespaces.front().front()->getBeginLoc(),
            "nested namespaces can be concatenated", DiagnosticIDs::Warning);
 
   SmallVector<SourceRange, 6> Fronts;
   Fronts.reserve(Namespaces.size() - 1U);
   SmallVector<SourceRange, 6> Backs;
   Backs.reserve(Namespaces.size());
 
-  NamespaceDecl const *LastNonNestND = nullptr;
-
-  for (const NamespaceDecl *ND : Namespaces) {
-    if (ND->isNested())
-      continue;
-    LastNonNestND = ND;
+  for (const NS &ND : Namespaces) {
     std::optional<SourceRange> SR =
-        getCleanedNamespaceFrontRange(ND, SM, LangOpts);
-    if (!SR.has_value())
+        ND.getCleanedNamespaceFrontRange(SM, LangOpts);
+    if (!SR)
       return;
     Fronts.push_back(SR.value());
-    Backs.push_back(getCleanedNamespaceBackRange(ND, SM, LangOpts));
+    Backs.push_back(ND.getNamespaceBackRange(SM, LangOpts));
   }
-  if (LastNonNestND == nullptr || Fronts.empty() || Backs.empty())
+  if (Fronts.empty() || Backs.empty())
     return;
+
   // the last one should be handled specially
   Fronts.pop_back();
   SourceRange LastRBrace = Backs.pop_back_val();
-  NamespaceString ConcatNameSpace = concatNamespaces();
+
+  NamespaceName ConcatNameSpace{"namespace "};
+  concatNamespace(ConcatNameSpace, Namespaces,
+                  [](const NS &NS) { return NS.getName(); });
 
   for (SourceRange const &Front : Fronts)
     DB << FixItHint::CreateRemoval(Front);
   DB << FixItHint::CreateReplacement(
-      SourceRange{LastNonNestND->getBeginLoc(),
-                  Namespaces.back()->getLocation()},
-      ConcatNameSpace);
-  if (LastRBrace !=
-      SourceRange{LastNonNestND->getRBraceLoc(), LastNonNestND->getRBraceLoc()})
+      Namespaces.back().getReplacedNamespaceFrontRange(), ConcatNameSpace);
+  if (LastRBrace != Namespaces.back().getDefaultNamespaceBackRange())
     DB << FixItHint::CreateReplacement(LastRBrace,
                                        ("} // " + ConcatNameSpace).str());
   for (SourceRange const &Back : llvm::reverse(Backs))
     DB << FixItHint::CreateRemoval(Back);
 }
 
 void ConcatNestedNamespacesCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   const NamespaceDecl &ND = *Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
   const SourceManager &Sources = *Result.SourceManager;
 
   if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
     return;
 
   if (unsupportedNamespace(ND))
     return;
 
-  Namespaces.push_back(&ND);
+  if (!ND.isNested())
+    Namespaces.push_back(NS{});
+  Namespaces.back().push_back(&ND);
 
   if (singleNamedNamespaceChild(ND))
     return;
 
-  SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
-                               Namespaces.back()->getLocation());
-
-  if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
-                           getLangOpts()))
+  if (Namespaces.size() > 1)
     reportDiagnostic(Sources, getLangOpts());
 
   Namespaces.clear();
 }
 
 } // namespace clang::tidy::modernize

clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp

 namespace avoid_change_close_comment {
 namespace inner {
 void foo() {}
 } // namespace inner and other comments
 } // namespace avoid_change_close_comment and other comments
 // CHECK-FIXES: namespace avoid_change_close_comment::inner {
 // CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner
 
+namespace /*::*/ comment_colon_1 {
+void foo() {}
+} // namespace comment_colon_1
+// CHECK-FIXES: namespace /*::*/ comment_colon_1 {
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace /*::*/ comment_colon_2 {
+namespace comment_colon_2 {
+void foo() {}
+} // namespace comment_colon_2
+} // namespace comment_colon_2
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
   n39::n40::t();
 #endif
 
 #ifdef IDONTEXIST
   n41::n42::t();
 #endif
 
   return 0;
 }