Author: Alexander Kornienko Date: 2019-12-03T20:30:41+01:00 New Revision: c375dc230d16f451cf1c7e2868237da6ef7bc4cf
URL: https://github.com/llvm/llvm-project/commit/c375dc230d16f451cf1c7e2868237da6ef7bc4cf DIFF: https://github.com/llvm/llvm-project/commit/c375dc230d16f451cf1c7e2868237da6ef7bc4cf.diff LOG: Revert "Fix llvm-namespace-comment for macro expansions" This reverts commit 4736d63f752f8d13f4c6a9afd558565c32119718. This commit introduces a ton of false positives and incorrect fixes. See https://reviews.llvm.org/D69855#1767089 for details. Added: Modified: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp Removed: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp ################################################################################ diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp index a2a56241e8ab..eb3d7c505b83 100644 --- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp @@ -19,44 +19,6 @@ namespace clang { namespace tidy { namespace readability { -namespace { -class NamespaceCommentPPCallbacks : public PPCallbacks { -public: - NamespaceCommentPPCallbacks(Preprocessor *PP, NamespaceCommentCheck *Check) - : PP(PP), Check(Check) {} - - void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) { - // Record all defined macros. We store the whole token to compare names - // later. - - const MacroInfo * MI = MD->getMacroInfo(); - - if (MI->isFunctionLike()) - return; - - std::string ValueBuffer; - llvm::raw_string_ostream Value(ValueBuffer); - - SmallString<128> SpellingBuffer; - bool First = true; - for (const auto &T : MI->tokens()) { - if (!First && T.hasLeadingSpace()) - Value << ' '; - - Value << PP->getSpelling(T, SpellingBuffer); - First = false; - } - - Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(), - Value.str()); - } - -private: - Preprocessor *PP; - NamespaceCommentCheck *Check; -}; -} // namespace - NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -78,37 +40,24 @@ void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher(namespaceDecl().bind("namespace"), this); } -void NamespaceCommentCheck::registerPPCallbacks( - const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { - PP->addPPCallbacks(std::make_unique<NamespaceCommentPPCallbacks>(PP, this)); -} - static bool locationsInSameFile(const SourceManager &Sources, SourceLocation Loc1, SourceLocation Loc2) { return Loc1.isFileID() && Loc2.isFileID() && Sources.getFileID(Loc1) == Sources.getFileID(Loc2); } -std::string NamespaceCommentCheck::getNamespaceComment(const NamespaceDecl *ND, - bool InsertLineBreak) { +static std::string getNamespaceComment(const NamespaceDecl *ND, + bool InsertLineBreak) { std::string Fix = "// namespace"; - if (!ND->isAnonymousNamespace()) { - bool IsNamespaceMacroExpansion; - StringRef MacroDefinition; - std::tie(IsNamespaceMacroExpansion, MacroDefinition) = - isNamespaceMacroExpansion(ND->getName()); - - Fix.append(" ").append(IsNamespaceMacroExpansion ? MacroDefinition - : ND->getName()); - } + if (!ND->isAnonymousNamespace()) + Fix.append(" ").append(ND->getNameAsString()); if (InsertLineBreak) Fix.append("\n"); return Fix; } -std::string -NamespaceCommentCheck::getNamespaceComment(const std::string &NameSpaceName, - bool InsertLineBreak) { +static std::string getNamespaceComment(const std::string &NameSpaceName, + bool InsertLineBreak) { std::string Fix = "// namespace "; Fix.append(NameSpaceName); if (InsertLineBreak) @@ -116,32 +65,6 @@ NamespaceCommentCheck::getNamespaceComment(const std::string &NameSpaceName, return Fix; } -void NamespaceCommentCheck::addMacro(const std::string &Name, - const std::string &Value) noexcept { - Macros.emplace_back(Name, Value); -} - -bool NamespaceCommentCheck::isNamespaceMacroDefinition( - const StringRef NameSpaceName) { - return llvm::any_of(Macros, [&NameSpaceName](const auto &Macro) { - return NameSpaceName == Macro.first; - }); -} - -std::tuple<bool, StringRef> NamespaceCommentCheck::isNamespaceMacroExpansion( - const StringRef NameSpaceName) { - const auto &MacroIt = - llvm::find_if(Macros, [&NameSpaceName](const auto &Macro) { - return NameSpaceName == Macro.second; - }); - - const bool IsNamespaceMacroExpansion = Macros.end() != MacroIt; - - return std::make_tuple(IsNamespaceMacroExpansion, - IsNamespaceMacroExpansion ? StringRef(MacroIt->first) - : NameSpaceName); -} - void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) { const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace"); const SourceManager &Sources = *Result.SourceManager; @@ -220,48 +143,28 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) { StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : ""; StringRef Anonymous = Groups.size() > 3 ? Groups[3] : ""; - // Don't allow to use macro expansion in closing comment. - // FIXME: Use Structured Bindings once C++17 features will be enabled. - bool IsNamespaceMacroExpansion; - StringRef MacroDefinition; - std::tie(IsNamespaceMacroExpansion, MacroDefinition) = - isNamespaceMacroExpansion(NamespaceNameInComment); - if (IsNested && NestedNamespaceName == NamespaceNameInComment) { // C++17 nested namespace. return; } else if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) || - (((ND->getNameAsString() == NamespaceNameInComment) && - Anonymous.empty()) && - !IsNamespaceMacroExpansion)) { + (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 diff erent format. return; } - // Allow using macro definitions in closing comment. - if (isNamespaceMacroDefinition(NamespaceNameInComment)) - return; - // Otherwise we need to fix the comment. NeedLineBreak = Comment.startswith("/*"); OldCommentRange = SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength())); - - if (IsNamespaceMacroExpansion) { - Message = (llvm::Twine("%0 ends with a comment that refers to an " - "expansion of macro")) - .str(); - NestedNamespaceName = MacroDefinition; - } else { - Message = (llvm::Twine("%0 ends with a comment that refers to a " - "wrong namespace '") + - NamespaceNameInComment + "'") - .str(); - } - + 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. @@ -274,16 +177,6 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) { // multi-line or there may be other tokens behind it. } - // Print Macro definition instead of expansion. - // FIXME: Use Structured Bindings once C++17 features will be enabled. - bool IsNamespaceMacroExpansion; - StringRef MacroDefinition; - std::tie(IsNamespaceMacroExpansion, MacroDefinition) = - isNamespaceMacroExpansion(NestedNamespaceName); - - if (IsNamespaceMacroExpansion) - NestedNamespaceName = MacroDefinition; - std::string NamespaceName = ND->isAnonymousNamespace() ? "anonymous namespace" diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h index bc5c11e7b71b..712cd4662965 100644 --- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h +++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h @@ -26,29 +26,14 @@ class NamespaceCommentCheck : public ClangTidyCheck { NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; - void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, - Preprocessor *ModuleExpanderPP) override; - - void addMacro(const std::string &Name, const std::string &Value) noexcept; private: void storeOptions(ClangTidyOptions::OptionMap &Options) override; - std::string getNamespaceComment(const NamespaceDecl *ND, - bool InsertLineBreak); - std::string getNamespaceComment(const std::string &NameSpaceName, - bool InsertLineBreak); - bool isNamespaceMacroDefinition(const StringRef NameSpaceName); - std::tuple<bool, StringRef> - isNamespaceMacroExpansion(const StringRef NameSpaceName); llvm::Regex NamespaceCommentPattern; const unsigned ShortNamespaceLines; const unsigned SpacesBeforeComments; llvm::SmallVector<SourceLocation, 4> Ends; - - // Store macros to verify that warning is not thrown when namespace name is a - // preprocessed define. - std::vector<std::pair<std::string, std::string>> Macros; }; } // namespace readability diff --git a/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp index b4e79c97c005..591c9dae5a74 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp @@ -25,10 +25,10 @@ void f(); // So that the namespace isn't empty. // 5 // 6 // 7 -// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with -// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here +// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with +// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here } -// CHECK-FIXES: } // namespace MACRO +// CHECK-FIXES: } // namespace macro_expansion namespace short1 { namespace short2 { diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp deleted file mode 100644 index a7d315693421..000000000000 --- a/clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp +++ /dev/null @@ -1,41 +0,0 @@ -// RUN: %check_clang_tidy %s llvm-namespace-comment %t - -namespace n1 { -namespace n2 { - void f(); - - - // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment] - // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment] -}} -// CHECK-FIXES: } // namespace n2 -// CHECK-FIXES: } // namespace n1 - -#define MACRO macro_expansion -namespace MACRO { - void f(); - // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment] -} -// CHECK-FIXES: } // namespace MACRO - -namespace MACRO { - void g(); -} // namespace MACRO - -namespace MACRO { - void h(); - // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'MACRO' ends with a comment that refers to an expansion of macro [llvm-namespace-comment] -} // namespace macro_expansion -// CHECK-FIXES: } // namespace MACRO - -namespace n1 { -namespace MACRO { -namespace n2 { - void f(); - // CHECK-MESSAGES: :[[@LINE+3]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment] - // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment] - // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment] -}}} -// CHECK-FIXES: } // namespace n2 -// CHECK-FIXES: } // namespace MACRO -// CHECK-FIXES: } // namespace n1 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
