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

alexfh · alexfh · commit 5a65e67d5dc3 · 2018-01-11T13:00:28.000Z
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. Reviewers: #clang-tools-extra, alexfh, aaron.ballman Reviewed By: aaron.ballman Subscribers: curdeius, cfe-commits, krasimir, JonasToth, JDevlieghere, xazax.hun Tags: #clang-tools-extra Patch by Alexandru Octavian Buțiu! Differential Revision: https://reviews.llvm.org/D38284 llvm-svn: 322274
diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -23,7 +23,7 @@ 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)) {}
@@ -56,6 +56,15 @@ static std::string getNamespaceComment(const NamespaceDecl *ND,
   return Fix;
 }
 
+static std::string getNamespaceComment(const std::string &NameSpaceName,
+                                       bool InsertLineBreak) {
+  std::string Fix = "// namespace ";
+  Fix.append(NameSpaceName);
+  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;
@@ -74,11 +83,44 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   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;
+  }
+
+  // Ignore macros
+  if (!ND->getLocation().isMacroID()) {
+    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());
+  StringRef NestedNamespaceName =
+      Lexer::getSourceText(TextRange, Sources, getLangOpts()).rtrim();
+  bool IsNested = NestedNamespaceName.contains(':');
+
+  if (IsNested)
+    Ends.push_back(LBracketLocation);
+  else
+    NestedNamespaceName = ND->getName();
+
   // 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;
 
@@ -98,10 +140,14 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
       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()) ||
-          (ND->getNameAsString() == NamespaceNameInComment &&
-           Anonymous.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;
@@ -131,13 +177,16 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   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(
+             CharSourceRange::getCharRange(OldCommentRange),
+             std::string(SpacesBeforeComments, ' ') +
+                 (IsNested
+                      ? getNamespaceComment(NestedNamespaceName, NeedLineBreak)
+                      : getNamespaceComment(ND, NeedLineBreak)));
   diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
       << NamespaceName;
 }
diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -34,6 +34,7 @@ class NamespaceCommentCheck : public ClangTidyCheck {
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
+  llvm::SmallVector<SourceLocation, 4> Ends;
 };
 
 } // namespace readability
diff --git a/clang-tools-extra/test/clang-tidy/google-readability-nested-namespace-comments.cpp b/clang-tools-extra/test/clang-tidy/google-readability-nested-namespace-comments.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
+
+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
+
