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

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry created this revision.
v1nh1shungry added reviewers: carlosgalvezp, Eugene.Zelenko.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
v1nh1shungry requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141787

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


Index: clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
@@ -156,6 +156,18 @@
 } // namespace n41
 // CHECK-FIXES: }
 
+namespace n43 {
+#define FOO
+namespace n44 {
+} // namespace n44
+} // namespace n43
+
+namespace n45 {
+namespace n46 {
+} // namespace n46
+#define BAR
+} // namespace n45
+
 int main() {
   n26::n27::n28::n29::n30::t();
 #ifdef IEXIST
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -24,13 +24,37 @@
   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,
@@ -76,6 +100,7 @@
     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;
@@ -85,7 +110,7 @@
 
   Namespaces.push_back(&ND);
 
-  if (singleNamedNamespaceChild(ND))
+  if (singleNamedNamespaceChild(ND, Sources, LangOpts))
     return;
 
   SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
@@ -94,7 +119,7 @@
                               Namespaces.front()->getRBraceLoc());
 
   if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
-                           getLangOpts()))
+                           LangOpts))
     reportDiagnostic(FrontReplacement, BackReplacement);
 
   Namespaces.clear();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141787.489339.patch
Type: text/x-patch
Size: 3460 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230115/7c0a959f/attachment.bin>