[clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces

[predator5047]

Summary

Fixes bug 34701

When we encounter a namespace find the location of the left bracket.
Then if the text between the name and the left bracket contains a ':'
then it's a C++17 nested namespace.

[JonasToth]

Could you add a test case for nested namespaces? I think it should be in a new file with C++17 enabled.

[krasimir]

clang-tidy/readability/NamespaceCommentCheck.cpp

60

Why there is an n at the end of namespacen?

[predator5047]

Fixed spelling mistake and added a test case.

[aaron.ballman]

clang-tidy/readability/NamespaceCommentCheck.cpp

87

Do not use auto here (the type is not spelled out in the initialization, despite being relatively obvious. Same below.

91

Missing a period at the end of the sentence.

92

i does not meet the usual naming conventions; you might want to pick a slightly better name.

93

You can elide the braces from the if statement.

110

Elide braces

136–143

Missing period in the comment

139–140

I'd move the comments inside of the braces (the braces can be elided, but if the comments are in the body instead of out of it, I think they're fine to leave).

clang-tidy/readability/NamespaceCommentCheck.h

37

Why 7?

test/clang-tidy/google-readability-nested-namespace-comments.cpp

7

Comment should start with a capital letter and end with punctuation.

9–15

Can remove the extra whitespace

18

Please make sure there's a newline at the end of the file.

[predator5047]

Address review comments:

• Don't use auto if the type is not spelled out in initialization.
• Better names for variables.
• Elide braces.
• Format changes.

[aaron.ballman]

clang-tidy/readability/NamespaceCommentCheck.cpp

59

s should be renamed to S or something more descriptive that meets the coding guidelines.

92

While you *can* elide the braces for the for loop, I think it looks a bit strange (we usually do it for if statements but not loops). You might want to put the braces back around the for loop body.

114

Same here.

clang-tidy/readability/NamespaceCommentCheck.h

37

I thought your use of SmallVector was appropriate, I was just wondering about the use of 7 as the expected size. I would have imagined we could get away with 2-4.

test/clang-tidy/google-readability-nested-namespace-comments.cpp

6

Space between // and the start of the comment.

[predator5047]

Address review comments:

• Use llvm::SmallVector instead of std::vector

• Some formatting changes.

[aaron.ballman]

Aside from a small nit, this LGTM, thanks!

clang-tidy/readability/NamespaceCommentCheck.cpp

102–105

These should not use auto since the type is not explicitly spelled out in the initialization.

[predator5047]

In D38284#888124, @aaron.ballman wrote:

Aside from a small nit, this LGTM, thanks!

Can you commit it? I don't have commit rights.

[aaron.ballman]

I've commit in r315057. Thank you!

[alexfh]

clang-tidy/readability/NamespaceCommentCheck.cpp

97

The check started triggering an assertion failure and incredible slowness (infinite loops?) on some real files. I've not yet come up with an isolated test case, but all this seems to be happening around this loop.

I'm going to revert this revision. A bug report (and hopefully a test case ;) will follow.

[alexfh]

clang-tidy/readability/NamespaceCommentCheck.cpp

97

Reverted in r315580.

[alexfh]

clang-tidy/readability/NamespaceCommentCheck.cpp

97

Here's a test case that demonstrates the issue:

#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]]:2: warning: namespace 'macro_expansion' not terminated with
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
}
// CHECK-FIXES: }  // namespace macro_expansion

I'll commit it once Subversion on llvm.org starts working again.

[alexfh]

clang-tidy/readability/NamespaceCommentCheck.cpp

97

Committed the regression test in r315682.

[predator5047]

Fixed assert error caused by macros.

[alexfh]

It looks like the latest patch was lost. I'll see whether it still applies cleanly...

clang-tidy/readability/NamespaceCommentCheck.h

public:
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
 
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
+  std::vector<SourceLocation> Ends;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NAMESPACECOMMENTCHECK_H

clang-tidy/readability/NamespaceCommentCheck.cpp

 namespace clang {
 namespace tidy {
 namespace readability {
 
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
-                              "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",
+                              "namespace( +([a-zA-Z0-9_:]+))?\\.? *(\\*/)?$",
                               llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
       SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
 }
static std::string getNamespaceComment(const NamespaceDecl *ND,
   std::string Fix = "// namespace";
   if (!ND->isAnonymousNamespace())
     Fix.append(" ").append(ND->getNameAsString());
   if (InsertLineBreak)
     Fix.append("\n");
   return Fix;
 }
 
+static std::string getNamespaceComment(const std::string &s,
+                                       bool InsertLineBreak) {
+  std::string Fix = "// namespace ";
+  Fix.append(s);
+  if (InsertLineBreak)
+    Fix.append("\n");
+  return Fix;
+}
+
 void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
   const SourceManager &Sources = *Result.SourceManager;
 
   if (!locationsInSameFile(Sources, ND->getLocStart(), ND->getRBraceLoc()))
     return;
 
   // Don't require closing comments for namespaces spanning less than certain
   // number of lines.
   unsigned StartLine = Sources.getSpellingLineNumber(ND->getLocStart());
   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 Loc = AfterRBrace;
   Token Tok;
+  SourceLocation LBracketLocation = ND->getLocation();
+  SourceLocation NestedNamespaceBegin = LBracketLocation;
+
+  // 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))
+      return;
+
+  while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
+         !Tok.is(tok::l_brace)) {
+    LBracketLocation = LBracketLocation.getLocWithOffset(1);
+  }
+
+  auto TextRange =
+      Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, LBracketLocation),
+                            Sources, getLangOpts());
+  auto NestedNamespaceName =
+      Lexer::getSourceText(TextRange, Sources, getLangOpts()).rtrim();
+  bool IsNested = NestedNamespaceName.contains(':');
+
+  if (IsNested)
+    Ends.push_back(LBracketLocation);
+
   // Skip whitespace until we find the next token.
   while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||
-         Tok.is(tok::semi)) {
+         Tok.is(tok::semi))
     Loc = Loc.getLocWithOffset(1);
-  }
+
   if (!locationsInSameFile(Sources, ND->getRBraceLoc(), Loc))
     return;
 
   bool NextTokenIsOnSameLine = Sources.getSpellingLineNumber(Loc) == EndLine;
   // If we insert a line comment before the token in the same line, we need
   // to insert a line break.
   bool NeedLineBreak = NextTokenIsOnSameLine && Tok.isNot(tok::eof);
 
   SourceRange OldCommentRange(AfterRBrace, AfterRBrace);
   std::string Message = "%0 not terminated with a closing comment";
 
   // 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] : "";
 
-      // Check if the namespace in the comment is the same.
-      if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
+      if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
+        // C++17 nested namespace.
+        return;
+      } else if ((ND->isAnonymousNamespace() &&
+                  NamespaceNameInComment.empty()) ||
                  (ND->getNameAsString() == NamespaceNameInComment &&
                   Anonymous.empty())) {
+        // 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("/*");
       OldCommentRange =
if (Tok.is(tok::comment) && NextTokenIsOnSameLine) {
     }
     // 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 '" + ND->getNameAsString() + "'");
+          : ("namespace '" + NestedNamespaceName.str() + "'");
 
   diag(AfterRBrace, Message)
-      << NamespaceName << FixItHint::CreateReplacement(
-                              CharSourceRange::getCharRange(OldCommentRange),
-                              std::string(SpacesBeforeComments, ' ') +
-                                  getNamespaceComment(ND, NeedLineBreak));
+      << NamespaceName
+      << FixItHint::CreateReplacement(
+                 (IsNested
+                      ? getNamespaceComment(NestedNamespaceName, NeedLineBreak)
+                      : getNamespaceComment(ND, NeedLineBreak)));
   diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
       << NamespaceName;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

test/clang-tidy/google-readability-nested-namespace-comments.cpp

+// RUN: %check_clang_tidy %s google-readability-namespace-comments %t -- -std=c++17
+
+namespace n1::n2 {
+namespace n3 {
+  
+  //So that namespace is not empty.
+  void f();
+  
+  
+// CHECK-MESSAGES: :[[@LINE+4]]:2: warning: namespace 'n3' not terminated with
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here
+// CHECK-MESSAGES: :[[@LINE+2]]:3: 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 n1::n2
+