[clang-tidy] Replace unrecognized namespace ending comments.

[alexfh]

Summary

Replace unrecognized namespace ending comments. This will help in particular when a namespace ending comment is mistyped or doesn't fit the regexp for other reason, e.g.:

namespace a {
namespace b {
namespace {
} // anoynmous namespace
} // b
} // namesapce a

[djasper]

Why do you need this as an option. Wouldn't it be the right thing to do in almost all cases?

[alexfh]

In D8078#134850, @djasper wrote:

Why do you need this as an option. Wouldn't it be the right thing to do in almost all cases?

There can actually be a meaningful comment in this place, and we're going to remove it with this option on. As this is a generic enough check, the way of allowing to configure this behavior seems to be preferred, I think. That doesn't make the code much more compilcated in any case.

[djasper]

I think adding this option is a way of copping out of fixing this correctly.

For now, I'd be fine saying always to overwrite such a comment. Either a project, enforcing such comment, then they won't have a different comment in the same place or they don't. If clang-tidy complains about / overwrites one of your comments, you'll reinstate it at a better location prior to submit.

A next step might be to analyze whether a comment in that place is meant to close the namespace, e.g. by checking whether its content contains at least one of "namespace" or <namespace-name>.

[alexfh]

Remove unrecognized namespace ending line comments unconditionally.

[djasper]

LG.

[alexfh]

In D8078#134852, @djasper wrote:

I think adding this option is a way of copping out of fixing this correctly.

For now, I'd be fine saying always to overwrite such a comment. Either a project, enforcing such comment, then they won't have a different comment in the same place or they don't. If clang-tidy complains about / overwrites one of your comments, you'll reinstate it at a better location prior to submit.

Frankly, the need for this option was not dictated by a specific request or an observation, rather an assumption that it may be needed. You're probably right that in this rare situation fixing up the change by hand is a proper solution and usually there's a better place for an unrelated comment than where a namespace ending comment should be.

A next step might be to analyze whether a comment in that place is meant to close the namespace, e.g. by checking whether its content contains at least one of "namespace" or <namespace-name>.

It may be not completely trivial to guess the intention, as the problem usually arises when there's a typo (namesapce, anoynmous, etc.). So we'd need to split the comment and look for keywords with a limit on the edit distance or do something similar. This may not worth it.

clang-tidy/readability/NamespaceCommentCheck.cpp

void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   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);
+  StringRef 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()) ||
           (ND->getNameAsString() == NamespaceNameInComment &&
            Anonymous.empty())) {
         // 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("/*");
-      CharSourceRange OldCommentRange = CharSourceRange::getCharRange(
-          SourceRange(Loc, Loc.getLocWithOffset(Tok.getLength())));
-      diag(Loc, "namespace closing comment refers to a wrong namespace '%0'")
-          << NamespaceNameInComment
-          << FixItHint::CreateReplacement(
-                 OldCommentRange, getNamespaceComment(ND, NeedLineBreak));
-      return;
-    }
-
-    // This is not a recognized form of a namespace closing comment.
-    // Leave line comment on the same line. Move block comment to the next line,
-    // as it can be multi-line or there may be other tokens behind it.
-    if (Comment.startswith("//"))
+      OldCommentRange =
+          SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength()));
+      Message =
+          (llvm::Twine(
+               "%0 ends with a comment that refers to a wrong namespace '") +
+           NamespaceNameInComment + "'").str();
+    } else if (Comment.startswith("//")) {
+      // Assume that this is an unrecognized form of a namespace closing line
+      // comment. Replace it.
       NeedLineBreak = false;
+      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 '" + ND->getNameAsString() + "'");
 
-  diag(AfterRBrace, "%0 not terminated with a closing comment")
+  diag(AfterRBrace, Message)
       << NamespaceName
-      << FixItHint::CreateInsertion(AfterRBrace,
-                                    std::string(SpacesBeforeComments, ' ') +
-                                        getNamespaceComment(ND, NeedLineBreak));
+      << FixItHint::CreateReplacement(
+             OldCommentRange, std::string(SpacesBeforeComments, ' ') +
   diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
       << NamespaceName;
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

unittests/clang-tidy/ReadabilityModuleTest.cpp

EXPECT_EQ("namespace My_NameSpace123 {\n"
                 "} //  end namespace   My_NameSpace123"));
   // Understand comments only on the same line.
   EXPECT_EQ("namespace {\n"
             "} // namespace\n"
             "// namespace",
             runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
                                                   "}\n"
                                                   "// namespace"));
-  // Leave unknown comments.
-  EXPECT_EQ("namespace {\n"
-            "} // namespace // random text",
-            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
-                                                  "} // random text"));
 }
 
 TEST(NamespaceCommentCheckTest, FixWrongComments) {
   EXPECT_EQ("namespace i { namespace jJ0_ {\n"
             "} // namespace jJ0_\n"
             " } // namespace i\n"
             " /* random comment */",
             runCheckOnCode<NamespaceCommentCheck>(
                 "namespace i { namespace jJ0_ {\n"
                 "} /* namespace qqq */ } /* random comment */"));
   EXPECT_EQ("namespace {\n"
             "} // namespace",
             runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
                                                   "} // namespace asdf"));
+  // Remove unknown comments.
+  EXPECT_EQ("namespace {\n"
+            "} // namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} // random text"));
 }
 
 TEST(BracesAroundStatementsCheck, IfWithComments) {
   EXPECT_EQ("int main() {\n"
             "  if (false /*dummy token*/) {\n"
             "    // comment\n"
             "    return -1; /**/\n"
             "}\n"