[clang-tidy] fix concat-nest-namespace fix hint remove the macro

[HerrCai0907]

Summary

Partially fixed #60035
This patch refactor the FixHint for concat-nest-namespace.

1. remove each namespace except the last non-nest namespace.
2. replace the last non-nest namespace with the new name.

It can remain the comment / pragma / macro between namespace and update the close comment.

[HerrCai0907]

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

72

This part of code copy from Lexer::findNextToken except this line. Should I refactor Lexer::findNextToken for example add a parameter to control it?
But Lexer::findNextToken is involved with lots of code. Maybe use another patch to do it?

[PiotrZSL]

You changed half of this check logic, please take a look into those issues for this check:
https://github.com/llvm/llvm-project/issues/60051
https://github.com/llvm/llvm-project/issues/60048
https://github.com/llvm/llvm-project/issues/60035
https://github.com/llvm/llvm-project/issues/57530
https://github.com/llvm/llvm-project/issues/56022
https://github.com/llvm/llvm-project/issues/43351
https://github.com/llvm/llvm-project/issues/41015
Add issues that you fixing to commit message.

Also look into these patches:
https://reviews.llvm.org/D61989
https://reviews.llvm.org/D141787

Update release notes (add info about fixes for this check).

Would be good to run it on some code to check how this behave, except that looks fine.

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

51–52

check if you can do something with this...

add test for:

with something like:

namespace x1 {
// namespace x3::x4 {
namespace x2 {
void foo();
}
// }
}

72

Dont change Lexer.

Look into clang-tools-extra/clang-tidy/utils/LexerUtils.h, if you can find something useful there,
if not then move this function there.

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

8

is check also fixes namespace comments ?

[PiotrZSL]

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

8

ok, neverming... lets let other check fix them...

[HerrCai0907]

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

51–52

It still have false negative.
But this should be done in another PR. In here I just extract a common function without changing the logic.

[HerrCai0907]

refactor some code and add release note

[PiotrZSL]

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

71

Remove this debug.

90

what if it is //namespace ? or /* namespace XYZ */

129

remove redundant {} from this function, they not needed for single line statements.

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

224

add same test but without spaces and move //CHECK-MESSAGES outside namespace.

[HerrCai0907]

add feature update close comment in fixhint
update acc. comment

[PiotrZSL]

Overall looks +- fine, these are last comments from me.

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

90

ok, but it still can be things like // namespace XYZ
So instead adding single space, consider using some trim/strip.
but thats minor issues, side effect would be just an leftover namespace comment.
add some test with something like // namespace XYZ - closing namespace to verify that some custom comments wont be removed, and make sure that documentation mention this, that we remove only namespace comments that follow some known standard.

you could also check for:

namespace C
{
} // C

129

this index looks reundant, you could use range-for

146–155

consider ordering removals from last to first...
personally i sometimes run into issues when we first removed something before last change, but that could be already fixed, or we may not run into this issue in this check

[HerrCai0907]

update according to comment

[HerrCai0907]

reorder fixhint

[HerrCai0907]

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

90

I think it can cover most of scenarios. It don't worth to introduce a mechanism to archive it.
Because something false positive like // name space XYZ should also be considered.

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

public:
   }
   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>;
 
-  void reportDiagnostic(const SourceRange &FrontReplacement,
-                        const SourceRange &BackReplacement);
+  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/Lex/Lexer.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() &&
          Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
 }
 
+static StringRef getRawStringRef(const SourceRange &Range,
+                                 const SourceManager &Sources,
+                                 const LangOptions &LangOpts) {
+  CharSourceRange TextRange = Lexer::getAsCharRange(Range, Sources, LangOpts);
+  return Lexer::getSourceText(TextRange, Sources, LangOpts);
+}
+
 static bool anonymousOrInlineNamespace(const NamespaceDecl &ND) {
   return ND.isAnonymousNamespace() || ND.isInlineNamespace();
 }
 
 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 && !anonymousOrInlineNamespace(*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.
-  CharSourceRange TextRange =
-      Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
-  StringRef CurrentNamespacesText =
-      Lexer::getSourceText(TextRange, Sources, LangOpts);
-  return CurrentNamespacesText.count(':') == (NumCandidates - 1) * 2;
+  return getRawStringRef(ReplacementRange, Sources, LangOpts).count(':') ==
+         (NumCandidates - 1) * 2;
+}
+
+static std::optional<SourceRange>
+getCleanedNamespaceFrontRange(const NamespaceDecl *ND, const SourceManager &SM,
+                              const LangOptions &LangOpts) {
+  // Front from namespace tp '{'
+  std::optional<Token> Tok =
+      ::clang::tidy::utils::lexer::findNextTokenSkippingComments(
+          ND->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()};
+}
+
+static SourceRange getCleanedNamespaceBackRange(const NamespaceDecl *ND,
+                                                const SourceManager &SM,
+                                                const LangOptions &LangOpts) {
+  // Back from '}' to conditional '// namespace xxx'
+  const SourceRange DefaultSourceRange =
+      SourceRange{ND->getRBraceLoc(), ND->getRBraceLoc()};
+  SourceLocation Loc = ND->getRBraceLoc();
+  std::optional<Token> Tok =
+      utils::lexer::findNextTokenIncludingComments(Loc, SM, LangOpts);
+  if (!Tok)
+    return DefaultSourceRange;
+  if (Tok->getKind() != tok::TokenKind::comment)
+    return DefaultSourceRange;
+  SourceRange TokRange = SourceRange{Tok->getLocation(), Tok->getEndLoc()};
+  StringRef TokText = getRawStringRef(TokRange, SM, LangOpts);
+  std::string CloseComment = "namespace " + ND->getNameAsString();
+  // current fix hint in readability/NamespaceCommentCheck.cpp use single line
+  // comment
+  if (TokText != "// " + CloseComment && TokText != "//" + CloseComment)
+    return DefaultSourceRange;
+  return SourceRange{ND->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;
 }
 
 void ConcatNestedNamespacesCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(ast_matchers::namespaceDecl().bind("namespace"), this);
 }
 
 void ConcatNestedNamespacesCheck::reportDiagnostic(
-    const SourceRange &FrontReplacement, const SourceRange &BackReplacement) {
+    const SourceManager &SM, const LangOptions &LangOpts) {
+  DiagnosticBuilder DB =
       diag(Namespaces.front()->getBeginLoc(),
-       "nested namespaces can be concatenated", DiagnosticIDs::Warning)
-      << FixItHint::CreateReplacement(FrontReplacement, concatNamespaces())
-      << FixItHint::CreateReplacement(BackReplacement, "}");
+           "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 *LastND = nullptr;
+
+  for (const NamespaceDecl *ND : Namespaces) {
+    if (ND->isNested())
+      continue;
+    LastND = ND;
+    std::optional<SourceRange> SR =
+        getCleanedNamespaceFrontRange(ND, SM, LangOpts);
+    if (!SR.has_value())
+      return;
+    Fronts.push_back(SR.value());
+    Backs.push_back(getCleanedNamespaceBackRange(ND, SM, LangOpts));
+  }
+  if (LastND == nullptr || Fronts.empty() || Backs.empty())
+    return;
+  // the last one should be handled specially
+  Fronts.pop_back();
+  SourceRange LastRBrace = Backs.pop_back_val();
+  NamespaceString ConcatNameSpace = concatNamespaces();
+
+  for (SourceRange const &Front : Fronts)
+    DB << FixItHint::CreateRemoval(Front);
+  DB << FixItHint::CreateReplacement(
+      SourceRange{LastND->getBeginLoc(), LastND->getLocation()},
+      ConcatNameSpace);
+  if (LastRBrace != SourceRange{LastND->getRBraceLoc(), LastND->getRBraceLoc()})
+    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 (anonymousOrInlineNamespace(ND))
     return;
 
   Namespaces.push_back(&ND);
 
   if (singleNamedNamespaceChild(ND))
     return;
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
                                Namespaces.back()->getLocation());
-  SourceRange BackReplacement(Namespaces.back()->getRBraceLoc(),
-                              Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
                            getLangOpts()))
-    reportDiagnostic(FrontReplacement, BackReplacement);
+    reportDiagnostic(Sources, getLangOpts());
 
   Namespaces.clear();
 }
 
 } // namespace clang::tidy::modernize

clang-tools-extra/clang-tidy/utils/LexerUtils.h

while (true) {
     // the loop, otherwise we will get infinite loop (findNextToken will return
     // eof on eof).
     if (PotentialMatch.is(tok::eof))
       return SourceLocation();
     Start = PotentialMatch.getLastLoc();
   }
 }
 
+std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+                               const LangOptions &LangOpts);
+
 // Finds next token that's not a comment.
 std::optional<Token> findNextTokenSkippingComments(SourceLocation Start,
                                                    const SourceManager &SM,
                                                    const LangOptions &LangOpts);
 
 /// Re-lex the provide \p Range and return \c false if either a macro spans
 /// multiple tokens, a pre-processor directive or failure to retrieve the
 /// next token is found, otherwise \c true.

clang-tools-extra/clang-tidy/utils/LexerUtils.cpp

 }
 
 SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
                                   const LangOptions &LangOpts) {
   return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
 }
 
 std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+                               const LangOptions &LangOpts) {
+  // `Lexer::findNextToken` will ignore comment
+  if (Start.isMacroID())
+    return std::nullopt;
+  Start = Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts);
+  // Break down the source location.
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Start);
+  bool InvalidTemp = false;
+  StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+  if (InvalidTemp)
+    return std::nullopt;
+  // Lex from the start of the given location.
+  Lexer L(SM.getLocForStartOfFile(LocInfo.first), LangOpts, File.begin(),
+          File.data() + LocInfo.second, File.end());
+  L.SetCommentRetentionState(true);
+  // Find the token.
+  Token Tok;
+  L.LexFromRawLexer(Tok);
+  return Tok;
+}
+
+std::optional<Token>
 findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM,
                               const LangOptions &LangOpts) {
   while (Start.isValid()) {
     std::optional<Token> CurrentToken =
         Lexer::findNextToken(Start, SM, LangOpts);
     if (!CurrentToken || !CurrentToken->is(tok::comment))
       return CurrentToken;
 

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed an issue in :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` when specifying an empty
   string for ``Prefix`` or ``Suffix`` options could result in the style not
   being used.
 
 - Fixed an issue in :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
   ``DISABLED_`` in the test suite name.
 
+- Fixed an issue in :doc:`modernize-concat-nested-namespaces
+  <clang-tidy/checks/modernize/concat-nested-namespaces>` when using macro between 
+  namespace declarations could result incorrect fix.
+
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer

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

 namespace nn1 {
 namespace nn2 {
 // CHECK-FIXES: namespace nn1::nn2
 void t();
 } // namespace nn2
 } // namespace nn1
 // CHECK-FIXES: void t();
-// CHECK-FIXES-NEXT: } // namespace nn1
+// CHECK-FIXES-NEXT: } // namespace nn1::nn2

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

 namespace n40 {
 // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
 // CHECK-FIXES: namespace n39::n40
 #ifdef IEXIST
 void t() {}
 #endif
 } // namespace n40
 } // namespace n39
-// CHECK-FIXES: }
+// CHECK-FIXES: } // namespace n39::n40
 
 namespace n41 {
 namespace n42 {
 // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
 // CHECK-FIXES: namespace n41::n42
 #ifdef IDONTEXIST
 void t() {}
 #endif
 } // namespace n42
 } // namespace n41
-// CHECK-FIXES: }
+// CHECK-FIXES: } // namespace n41::n42
+
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace n43 {
+#define N43_INNER
+namespace n44 {
+void foo() {}
+} // namespace n44
+#undef N43_INNER
+} // namespace n43
+// CHECK-FIXES: #define N43_INNER
+// CHECK-FIXES: namespace n43::n44 {
+// CHECK-FIXES: } // namespace n43::n44
+// CHECK-FIXES: #undef N43_INNER
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace n45{
+#define N45_INNER
+namespace n46
+{
+#pragma clang diagnostic push
+namespace n47 {
+void foo() {}
+} // namespace n47
+#pragma clang diagnostic pop
+} //namespace n46
+#undef N45_INNER
+} //namespace n45
+// CHECK-FIXES: #define N45_INNER
+// CHECK-FIXES: #pragma clang diagnostic push
+// CHECK-FIXES: namespace n45::n46::n47 {
+// CHECK-FIXES: } // namespace n45::n46::n47
+// CHECK-FIXES: #pragma clang diagnostic pop
+// CHECK-FIXES: #undef N45_INNER
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+namespace avoid_add_close_comment {
+namespace inner {
+void foo() {}
+}
+}
+// CHECK-FIXES: namespace avoid_add_close_comment::inner {
+// CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner
+
+// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+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
 
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
   n39::n40::t();
 #endif
 
 #ifdef IDONTEXIST
   n41::n42::t();
 #endif
 
   return 0;
 }