[clang-tidy] fix a false positive of `modernize-concat-nested-namespaces`

[v1nh1shungry]

Summary

Currently this check will complain when there're preprocessor directives
like macro definitions between the namespaces, e.g.

namespace a { // warns, but it shouldn't
 #define FOO
namespace b {
} // namespace b
} // namespace a

Fixes https://github.com/llvm/llvm-project/issues/60035 partly

[v1nh1shungry]

This patch doesn't handle situations like:

namespace a {
namespace b {
#define FOO
namespace c {
}
}
}

[v1nh1shungry]

add a test

[v1nh1shungry]

In D141787#4054514, @v1nh1shungry wrote:

This patch doesn't handle situations like:

namespace a {
namespace b {
#define FOO
namespace c {
}
}
}

It did. I must be mad :(

[njames93]

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

173

Is CHECK-MESSAGES-DAG needed here, why does it fail if this is omitted?

[v1nh1shungry]

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

173

Hmm, doesn't CHECK-MESSAGES-DAG means to check whether there is a message? (I referred to the tests above.)

If it's other cases, maybe it's due to https://github.com/llvm/llvm-project/issues/60051.

I'm not sure I understand what you mean, sorry!

[v1nh1shungry]

Ping? @njames93

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

173

Hmm, doesn't CHECK-MESSAGES-DAG means to check whether there is a message? (I referred to the tests above.)

If it's other cases, maybe it's due to https://github.com/llvm/llvm-project/issues/60051.

I'm not sure I understand what you mean, sorry!

I think I understand now. You mean the whole test doesn't fail even without this CHECK-MESSAGES-DAG, right?

If so, this CHECK-MESSAGES-DAG is needed. I think the reason that the test doesn't fail without it is https://github.com/llvm/llvm-project/issues/60051. The check isn't applied correctly before I make any code changes, you can have a play with https://godbolt.org/z/nc34shM8W. But I think the problem isn't related to the check itself because when I apply this check through clangd's code action it produces correct codes as I mentioned in the GitHub issue.

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

static bool locationsInSameFile(const SourceManager &Sources,
   return Loc1.isFileID() && Loc2.isFileID() &&
          Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
 }
 
 static bool anonymousOrInlineNamespace(const NamespaceDecl &ND) {
   return ND.isAnonymousNamespace() || ND.isInlineNamespace();
 }
 
-static bool singleNamedNamespaceChild(const NamespaceDecl &ND) {
+static bool hasPPDirective(SourceRange Range, const SourceManager &SM,
+                           const LangOptions &LangOpts) {
+  CharSourceRange CharRange =
+      Lexer::makeFileCharRange({Range, true}, SM, LangOpts);
+  StringRef BetweenText = Lexer::getSourceText(CharRange, SM, LangOpts);
+  std::string Buffer{BetweenText};
+  Lexer Lex(Range.getBegin(), LangOpts, Buffer.c_str(), Buffer.c_str(),
+            Buffer.c_str() + Buffer.size());
+  Token Tok;
+  while (!Lex.LexFromRawLexer(Tok)) {
+    if (Tok.getKind() == tok::hash)
+      return true;
+  }
+  return false;
+}
+
+static bool singleNamedNamespaceChild(const NamespaceDecl &ND,
+                                      const SourceManager &SM,
+                                      const LangOptions &LangOpts) {
   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);
+  if (!ChildNamespace || anonymousOrInlineNamespace(*ChildNamespace))
+    return false;
+
+  return !hasPPDirective({ND.getBeginLoc(), ChildNamespace->getBeginLoc()}, SM,
+                         LangOpts) &&
+         !hasPPDirective({ChildNamespace->getRBraceLoc(), ND.getRBraceLoc()},
+                         SM, LangOpts);
 }
 
 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 =
diag(Namespaces.front()->getBeginLoc(),
       << FixItHint::CreateReplacement(FrontReplacement, concatNamespaces())
       << FixItHint::CreateReplacement(BackReplacement, "}");
 }
 
 void ConcatNestedNamespacesCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   const NamespaceDecl &ND = *Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
   const SourceManager &Sources = *Result.SourceManager;
+  const LangOptions &LangOpts = getLangOpts();
 
   if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
     return;
 
   if (anonymousOrInlineNamespace(ND))
     return;
 
   Namespaces.push_back(&ND);
 
-  if (singleNamedNamespaceChild(ND))
+  if (singleNamedNamespaceChild(ND, Sources, LangOpts))
     return;
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
                                Namespaces.back()->getLocation());
   SourceRange BackReplacement(Namespaces.back()->getRBraceLoc(),
                               Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
-                           getLangOpts()))
+                           LangOpts))
     reportDiagnostic(FrontReplacement, BackReplacement);
 
   Namespaces.clear();
 }
 
 } // namespace clang::tidy::modernize

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

 // CHECK-FIXES: namespace n41::n42
 #ifdef IDONTEXIST
 void t() {}
 #endif
 } // namespace n42
 } // namespace n41
 // CHECK-FIXES: }
 
+namespace n43 {
+#define FOO
+namespace n44 {
+} // namespace n44
+} // namespace n43
+
+namespace n45 {
+namespace n46 {
+} // namespace n46
+#define BAR
+} // namespace n45
+
+namespace n47 {
+namespace n48 {
+// CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n47::n48
+#define FOOBAR
+namespace n49 {
+} // namespace n49
+} // namespace n48
+} // namespace n47
+// CHECK-FIXES: }
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
   n39::n40::t();
 #endif
 
 #ifdef IDONTEXIST
   n41::n42::t();
 #endif
 
   return 0;
 }