[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.h

public:
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
 
   llvm::Regex NamespaceCommentPattern;
+  const bool ReplaceUnknownComments;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NAMESPACECOMMENTCHECK_H

clang-tidy/readability/NamespaceCommentCheck.cpp

 namespace readability {
 
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
                               "namespace( +([a-zA-Z0-9_]+))?\\.? *(\\*/)?$",
                               llvm::Regex::IgnoreCase),
+      ReplaceUnknownComments(Options.get("ReplaceUnknownComments", "false") ==
+                             "true"),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
       SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  // TODO: Implement (de-)serialization of bool options in the core library.
+  Options.store(Opts, "ReplaceUnknownComments",
+                ReplaceUnknownComments ? "true" : "false");
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 }
 
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;
-    }
+      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 if needed.
 
-    // 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("//"))
       NeedLineBreak = false;
+      if (ReplaceUnknownComments) {
+        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/ClangTidyTest.h

private:
   }
 
   ClangTidyCheck &Check;
   ast_matchers::MatchFinder &Finder;
   ClangTidyContext &Context;
 };
 
 template <typename T>
-std::string runCheckOnCode(StringRef Code,
-                           std::vector<ClangTidyError> *Errors = nullptr,
-                           const Twine &Filename = "input.cc",
-                           ArrayRef<std::string> ExtraArgs = None) {
-  ClangTidyOptions Options;
-  Options.Checks = "*";
+std::string
+runCheckWithOptionsOnCode(const ClangTidyOptions &Options, StringRef Code,
+                          const Twine &FileName = "input.cc",
+
   ClangTidyContext Context(llvm::make_unique<DefaultOptionsProvider>(
       ClangTidyGlobalOptions(), Options));
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
   T Check("test-check", &Context);
   ast_matchers::MatchFinder Finder;
   Check.registerMatchers(&Finder);
 
   std::vector<std::string> ArgCXX11(1, "clang-tidy");
   ArgCXX11.push_back("-fsyntax-only");
   ArgCXX11.push_back("-std=c++11");
   ArgCXX11.insert(ArgCXX11.end(), ExtraArgs.begin(), ExtraArgs.end());
-  ArgCXX11.push_back(Filename.str());
+  ArgCXX11.push_back(FileName.str());
   llvm::IntrusiveRefCntPtr<FileManager> Files(
       new FileManager(FileSystemOptions()));
   tooling::ToolInvocation Invocation(
       ArgCXX11, new TestClangTidyAction(Check, Finder, Context), Files.get());
-  Invocation.mapVirtualFile(Filename.str(), Code);
+  Invocation.mapVirtualFile(FileName.str(), Code);
   Invocation.setDiagnosticConsumer(&DiagConsumer);
   if (!Invocation.run())
     return "";
 
   DiagConsumer.finish();
   tooling::Replacements Fixes;
   for (const ClangTidyError &Error : Context.getErrors())
     Fixes.insert(Error.Fix.begin(), Error.Fix.end());
   if (Errors)
     *Errors = Context.getErrors();
   return tooling::applyAllReplacements(Code, Fixes);
 }
 
+template <typename T>
+std::string runCheckOnCode(StringRef Code,
+                           std::vector<ClangTidyError> *Errors = nullptr,
+                           const Twine &FileName = "input.cc",
+                           ArrayRef<std::string> ExtraArgs = None) {
+  ClangTidyOptions Options;
+  Options.Checks = "*";
+  return runCheckWithOptionsOnCode<T>(Options, Code, Errors, FileName,
+                                      ExtraArgs);
+}
+
 #define EXPECT_NO_CHANGES(Check, Code)                                         \
   EXPECT_EQ(Code, runCheckOnCode<Check>(Code))
 
 } // namespace test
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANG_TIDY_TEST_H

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"));
+  ClangTidyOptions DontReplaceUnknownComments;
+  DontReplaceUnknownComments.Checks = "*";
+  DontReplaceUnknownComments.CheckOptions["test-check.ReplaceUnknownComments"] =
+      "false";
+  // By default, don't remove unknown comments.
+  EXPECT_EQ("namespace {\n"
+            "} // namespace // random text",
+            runCheckWithOptionsOnCode<NamespaceCommentCheck>(
+                DontReplaceUnknownComments, "namespace {\n"
+                                            "} // random text"));
+  ClangTidyOptions ReplaceUnknownComments;
+  ReplaceUnknownComments.Checks = "*";
+  ReplaceUnknownComments.CheckOptions["test-check.ReplaceUnknownComments"] =
+      "true";
+  // Remove unknown comments.
+  EXPECT_EQ("namespace {\n"
+            "} // namespace",
+            runCheckWithOptionsOnCode<NamespaceCommentCheck>(
+                ReplaceUnknownComments, "namespace {\n"
+                                        "} // random text"));
 }
 
 TEST(BracesAroundStatementsCheck, IfWithComments) {
   EXPECT_EQ("int main() {\n"
             "  if (false /*dummy token*/) {\n"
             "    // comment\n"
             "    return -1; /**/\n"
             "}\n"