Index: clang-tidy/readability/NamespaceCommentCheck.h =================================================================== --- clang-tidy/readability/NamespaceCommentCheck.h +++ clang-tidy/readability/NamespaceCommentCheck.h @@ -31,6 +31,7 @@ void storeOptions(ClangTidyOptions::OptionMap &Options) override; llvm::Regex NamespaceCommentPattern; + const bool ReplaceUnknownComments; const unsigned ShortNamespaceLines; const unsigned SpacesBeforeComments; }; Index: clang-tidy/readability/NamespaceCommentCheck.cpp =================================================================== --- clang-tidy/readability/NamespaceCommentCheck.cpp +++ clang-tidy/readability/NamespaceCommentCheck.cpp @@ -25,10 +25,15 @@ 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); } @@ -82,6 +87,9 @@ // 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()); @@ -101,20 +109,25 @@ // 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 = @@ -122,11 +135,11 @@ ? "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, ' ') + + getNamespaceComment(ND, NeedLineBreak)); diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note) << NamespaceName; } Index: unittests/clang-tidy/ClangTidyTest.h =================================================================== --- unittests/clang-tidy/ClangTidyTest.h +++ unittests/clang-tidy/ClangTidyTest.h @@ -42,12 +42,12 @@ }; template -std::string runCheckOnCode(StringRef Code, - std::vector *Errors = nullptr, - const Twine &Filename = "input.cc", - ArrayRef ExtraArgs = None) { - ClangTidyOptions Options; - Options.Checks = "*"; +std::string +runCheckWithOptionsOnCode(const ClangTidyOptions &Options, StringRef Code, + std::vector *Errors = nullptr, + const Twine &FileName = "input.cc", + ArrayRef ExtraArgs = None) { + ClangTidyContext Context(llvm::make_unique( ClangTidyGlobalOptions(), Options)); ClangTidyDiagnosticConsumer DiagConsumer(Context); @@ -59,12 +59,12 @@ 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 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 ""; @@ -78,6 +78,17 @@ return tooling::applyAllReplacements(Code, Fixes); } +template +std::string runCheckOnCode(StringRef Code, + std::vector *Errors = nullptr, + const Twine &FileName = "input.cc", + ArrayRef ExtraArgs = None) { + ClangTidyOptions Options; + Options.Checks = "*"; + return runCheckWithOptionsOnCode(Options, Code, Errors, FileName, + ExtraArgs); +} + #define EXPECT_NO_CHANGES(Check, Code) \ EXPECT_EQ(Code, runCheckOnCode(Code)) Index: unittests/clang-tidy/ReadabilityModuleTest.cpp =================================================================== --- unittests/clang-tidy/ReadabilityModuleTest.cpp +++ unittests/clang-tidy/ReadabilityModuleTest.cpp @@ -75,11 +75,6 @@ runCheckOnCode("namespace {\n" "}\n" "// namespace")); - // Leave unknown comments. - EXPECT_EQ("namespace {\n" - "} // namespace // random text", - runCheckOnCode("namespace {\n" - "} // random text")); } TEST(NamespaceCommentCheckTest, FixWrongComments) { @@ -94,6 +89,26 @@ "} // namespace", runCheckOnCode("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( + DontReplaceUnknownComments, "namespace {\n" + "} // random text")); + ClangTidyOptions ReplaceUnknownComments; + ReplaceUnknownComments.Checks = "*"; + ReplaceUnknownComments.CheckOptions["test-check.ReplaceUnknownComments"] = + "true"; + // Remove unknown comments. + EXPECT_EQ("namespace {\n" + "} // namespace", + runCheckWithOptionsOnCode( + ReplaceUnknownComments, "namespace {\n" + "} // random text")); } TEST(BracesAroundStatementsCheck, IfWithComments) {