[clang-tidy] Fix PR26274

[alexfh]

Summary

This commit fixes http://llvm.org/PR26274 in a simpler and more correct way than
4736d63f752f8d13f4c6a9afd558565c32119718 did. See
https://reviews.llvm.org/D69855#1767089 for details.

[merge_guards_bot]

Build result: pass - 60438 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[aaron.ballman]

Thank you for working on this!

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

90–92

accross -> across

124

Will this work okay with inline namespaces, or namespaces with attributes written on them? May want test cases for those situations.

[gribozavr2]

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

[aaron.ballman]

In D70974#1768871, @gribozavr2 wrote:

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

While I think that's a superior solution to using macros, some users have macros instead. This fixes a bug reported in https://bugs.llvm.org/show_bug.cgi?id=26274 and I agree that the behavior described in that bug is not what I would expect it to be.

[gribozavr2]

In D70974#1768902, @aaron.ballman wrote:

In D70974#1768871, @gribozavr2 wrote:

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

While I think that's a superior solution to using macros, some users have macros instead. This fixes a bug reported in https://bugs.llvm.org/show_bug.cgi?id=26274 and I agree that the behavior described in that bug is not what I would expect it to be.

I mean, it is possible to break pretty much any ClangTidy check with sneaky code. But there's a limit to which we should try to make things work in tricky corner cases. For example, the fix in this patch does not handle function-like macros (namespace MY_LIBRARY_NAMESPACE_FOR_VERSION(42) {).

LGTM if we must... but I don't think we should.

[alexfh]

• Added support for inline namespaces and namespace attributes, fixed a typo, added tests.

[merge_guards_bot]

Build result: pass - 60438 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[alexfh]

In D70974#1769036, @gribozavr2 wrote:

In D70974#1768902, @aaron.ballman wrote:

In D70974#1768871, @gribozavr2 wrote:

I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

While I think that's a superior solution to using macros, some users have macros instead. This fixes a bug reported in https://bugs.llvm.org/show_bug.cgi?id=26274 and I agree that the behavior described in that bug is not what I would expect it to be.

I mean, it is possible to break pretty much any ClangTidy check with sneaky code. But there's a limit to which we should try to make things work in tricky corner cases. For example, the fix in this patch does not handle function-like macros (namespace MY_LIBRARY_NAMESPACE_FOR_VERSION(42) {).

LGTM if we must... but I don't think we should.

Apart from preprocessor-related stuff, this patch improves support for nested inline namespace declarations (namespace a::inline b::c), attributes on namespaces and comments inside namespace declarations. And also reduces the number of special cases. It should be a net positive effect ;)

[gribozavr2]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

54

s/begin/beginning/

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

79

How well do these test cases work?

namespace [[]] {
}

namespace frobble::inline bar {
}

I'm not certain what we want the comment to be for the second example, and I'm not certain the first example will parse properly here.

[alexfh]

• Added a test with an empty attribute list.

[alexfh]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

79

Both work correctly, in the second case the inline is dropped (why would we want it not to?). See clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp:66 and clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp:4 for tests covering very similar cases. Just in case, I've added a test with an empty attribute list.

[merge_guards_bot]

Build result: pass - 60438 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[aaron.ballman]

LGTM, thanks for the fixes!

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

 //===--- NamespaceCommentCheck.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 "NamespaceCommentCheck.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringExtras.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {

 }
 
 static bool locationsInSameFile(const SourceManager &Sources,
                                 SourceLocation Loc1, SourceLocation Loc2) {
   return Loc1.isFileID() && Loc2.isFileID() &&
          Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
 }
 
-static std::string getNamespaceComment(const NamespaceDecl *ND,
-                                       bool InsertLineBreak) {
-  std::string Fix = "// namespace";
-  if (!ND->isAnonymousNamespace())
-    Fix.append(" ").append(ND->getNameAsString());
-  if (InsertLineBreak)
-    Fix.append("\n");
-  return Fix;
+static llvm::Optional<std::string>
+getNamespaceNameAsWritten(SourceLocation &Loc, const SourceManager &Sources,
+                          const LangOptions &LangOpts) {
+  // Loc should be at the begin of the namespace decl (usually, `namespace`
+  // token). We skip the first token right away, but in case of `inline
+  // namespace` or `namespace a::inline b` we can see both `inline` and
+  // `namespace` keywords, which we just ignore. Nested parens/squares before
+  // the opening brace can result from attributes.
+  std::string Result;
+  int Nesting = 0;
+  while (llvm::Optional<Token> T = utils::lexer::findNextTokenSkippingComments(
+             Loc, Sources, LangOpts)) {
+    Loc = T->getLocation();
+    if (T->is(tok::l_brace))
+      break;
+
+    if (T->isOneOf(tok::l_square, tok::l_paren)) {
+      ++Nesting;
+    } else if (T->isOneOf(tok::r_square, tok::r_paren)) {
+      --Nesting;
+    } else if (Nesting == 0) {
+      if (T->is(tok::raw_identifier)) {
+        StringRef ID = T->getRawIdentifier();
+        if (ID != "namespace" && ID != "inline")
+          Result.append(ID);
+      } else if (T->is(tok::coloncolon)) {
+        Result.append("::");
+      } else { // Any other kind of token is unexpected here.
+        return llvm::None;
+      }
     }
-
-static std::string getNamespaceComment(const std::string &NameSpaceName,
-                                       bool InsertLineBreak) {
-  std::string Fix = "// namespace ";
-  Fix.append(NameSpaceName);
-  if (InsertLineBreak)
-    Fix.append("\n");
-  return Fix;
+  }
+  return Result;
 }
 
 void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
   const SourceManager &Sources = *Result.SourceManager;
 
-  if (!locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
+  // Ignore namespaces inside macros and namespaces split across files.
+  if (ND->getBeginLoc().isMacroID() ||
+      !locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
     return;
 
   // Don't require closing comments for namespaces spanning less than certain
   // number of lines.
   unsigned StartLine = Sources.getSpellingLineNumber(ND->getBeginLoc());
   unsigned EndLine = Sources.getSpellingLineNumber(ND->getRBraceLoc());
   if (EndLine - StartLine + 1 <= ShortNamespaceLines)
     return;
 
   // Find next token after the namespace closing brace.
-  SourceLocation AfterRBrace = ND->getRBraceLoc().getLocWithOffset(1);
+  SourceLocation AfterRBrace = Lexer::getLocForEndOfToken(
+      ND->getRBraceLoc(), /*Offset=*/0, Sources, getLangOpts());
   SourceLocation Loc = AfterRBrace;
-  Token Tok;
-  SourceLocation LBracketLocation = ND->getLocation();
-  SourceLocation NestedNamespaceBegin = LBracketLocation;
+  SourceLocation LBraceLoc = ND->getBeginLoc();
 
   // Currently for nested namepsace (n1::n2::...) the AST matcher will match foo
   // then bar instead of a single match. So if we got a nested namespace we have
   // to skip the next ones.
   for (const auto &EndOfNameLocation : Ends) {
-    if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin,
-                                          EndOfNameLocation))
+    if (Sources.isBeforeInTranslationUnit(ND->getLocation(), EndOfNameLocation))
       return;
   }
 
-  // Ignore macros
-  if (!ND->getLocation().isMacroID()) {
-    while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
-           !Tok.is(tok::l_brace)) {
-      LBracketLocation = LBracketLocation.getLocWithOffset(1);
-    }
-  }
+  llvm::Optional<std::string> NamespaceNameAsWritten =
+      getNamespaceNameAsWritten(LBraceLoc, Sources, getLangOpts());
+  if (!NamespaceNameAsWritten)
+    return;
 
-  // FIXME: This probably breaks on comments between the namespace and its '{'.
-  auto TextRange =
-      Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, LBracketLocation),
-                            Sources, getLangOpts());
-  StringRef NestedNamespaceName =
-      Lexer::getSourceText(TextRange, Sources, getLangOpts())
-          .rtrim('{') // Drop the { itself.
-          .rtrim();   // Drop any whitespace before it.
-  bool IsNested = NestedNamespaceName.contains(':');
+  if (NamespaceNameAsWritten->empty() != ND->isAnonymousNamespace()) {
+    // Apparently, we didn't find the correct namespace name. Give up.
+    return;
+  }
 
-  if (IsNested)
-    Ends.push_back(LBracketLocation);
-  else
-    NestedNamespaceName = ND->getName();
+  Ends.push_back(LBraceLoc);
 
+  Token Tok;
   // Skip whitespace until we find the next token.
   while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||
          Tok.is(tok::semi)) {
     Loc = Loc.getLocWithOffset(1);
   }
 
   if (!locationsInSameFile(Sources, ND->getRBraceLoc(), Loc))
     return;
void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   // Try to find existing namespace closing comment on the same line.
   if (Tok.is(tok::comment) && NextTokenIsOnSameLine) {
     StringRef Comment(Sources.getCharacterData(Loc), Tok.getLength());
     SmallVector<StringRef, 7> Groups;
     if (NamespaceCommentPattern.match(Comment, &Groups)) {
       StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
       StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";
 
-      if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
-        // C++17 nested namespace.
-        return;
-      } else if ((ND->isAnonymousNamespace() &&
-                  NamespaceNameInComment.empty()) ||
-                 (ND->getNameAsString() == NamespaceNameInComment &&
-                  Anonymous.empty())) {
+      if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
+          (*NamespaceNameAsWritten == NamespaceNameInComment &&
         // Check if the namespace in the comment is the same.
         // FIXME: Maybe we need a strict mode, where we always fix namespace
         // comments with different format.
         return;
       }
 
       // Otherwise we need to fix the comment.
       NeedLineBreak = Comment.startswith("/*");
if (NamespaceCommentPattern.match(Comment, &Groups)) {
       OldCommentRange =
           SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength()));
       Message = "%0 ends with an unrecognized comment";
     }
     // If it's a block comment, just move it to the next line, as it can be
     // multi-line or there may be other tokens behind it.
   }
 
-  std::string NamespaceName =
-      ND->isAnonymousNamespace()
-          ? "anonymous namespace"
-          : ("namespace '" + NestedNamespaceName.str() + "'");
+  std::string NamespaceNameForDiag =
+      ND->isAnonymousNamespace() ? "anonymous namespace"
+                                 : ("namespace '" + *NamespaceNameAsWritten + "'");
+
+  std::string Fix(SpacesBeforeComments, ' ');
+  Fix.append("// namespace");
+  if (!ND->isAnonymousNamespace())
+    Fix.append(" ").append(*NamespaceNameAsWritten);
+  if (NeedLineBreak)
+    Fix.append("\n");
 
   // Place diagnostic at an old comment, or closing brace if we did not have it.
   SourceLocation DiagLoc =
       OldCommentRange.getBegin() != OldCommentRange.getEnd()
           ? OldCommentRange.getBegin()
           : ND->getRBraceLoc();
 
-  diag(DiagLoc, Message)
-      << NamespaceName
+  diag(DiagLoc, Message) << NamespaceNameForDiag
                          << FixItHint::CreateReplacement(
                                 CharSourceRange::getCharRange(OldCommentRange),
-             std::string(SpacesBeforeComments, ' ') +
-                 (IsNested
-                      ? getNamespaceComment(NestedNamespaceName, NeedLineBreak)
-                      : getNamespaceComment(ND, NeedLineBreak)));
+                                Fix);
   diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
-      << NamespaceName;
+      << NamespaceNameForDiag;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp

 // RUN: %check_clang_tidy %s google-readability-namespace-comments %t
 
 namespace n1::n2 {
-namespace n3 {
+namespace /*comment1*/n3/*comment2*/::/*comment3*/inline/*comment4*/n4/*comment5*/ {
 
 // So that namespace is not empty.
 void f();
 
 
-// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3' not terminated with
-// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here
+// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3::n4' not terminated with
+// CHECK-MESSAGES: :[[@LINE-7]]:23: note: namespace 'n3::n4' starts here
 // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1::n2' not terminated with a closing comment [google-readability-namespace-comments]
 // CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts here
 }}
-// CHECK-FIXES: }  // namespace n3
+// CHECK-FIXES: }  // namespace n3::n4
 // CHECK-FIXES: }  // namespace n1::n2
 

clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp

 // RUN: %check_clang_tidy %s google-readability-namespace-comments %t
 
 namespace n1 {
-namespace n2 {
+namespace /* a comment */ n2 /* another comment */ {
 
 
 void f(); // So that the namespace isn't empty.
 
 
 // CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n2' not terminated with a closing comment [google-readability-namespace-comments]
-// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n2' starts here
+// CHECK-MESSAGES: :[[@LINE-7]]:27: note: namespace 'n2' starts here
 // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1' not terminated with
 // CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1' starts here
 }}
 // CHECK-FIXES: }  // namespace n2
 // CHECK-FIXES: }  // namespace n1
 
 #define MACRO macro_expansion
 namespace MACRO {
 void f(); // So that the namespace isn't empty.
 // 1
 // 2
 // 3
 // 4
 // 5
 // 6
 // 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
+}
+// CHECK-FIXES: }  // namespace MACRO
+
+namespace macro_expansion {
+void ff(); // So that the namespace isn't empty.
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
 // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
 // CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
 }
 // CHECK-FIXES: }  // namespace macro_expansion
 
+namespace [[deprecated("foo")]] namespace_with_attr {
+inline namespace inline_namespace {
+void g();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'inline_namespace' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:18: note: namespace 'inline_namespace' starts here
+}
+// CHECK-FIXES: }  // namespace inline_namespace
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'namespace_with_attr' not terminated with
+// CHECK-MESSAGES: :[[@LINE-15]]:33: note: namespace 'namespace_with_attr' starts here
+}
+// CHECK-FIXES: }  // namespace namespace_with_attr
+
+namespace [[deprecated]] {
+void h();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:26: note: anonymous namespace starts here
+}
+// CHECK-FIXES: }  // namespace{{$}}
+
+namespace [[]] {
+void hh();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:16: note: anonymous namespace starts here
+}
+// CHECK-FIXES: }  // namespace{{$}}
+
 namespace short1 {
 namespace short2 {
 // Namespaces covering 10 lines or fewer are exempt from this rule.