diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp index eb3d7c505b831..a2a56241e8ab6 100644 --- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp @@ -19,6 +19,44 @@ 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), @@ -40,24 +78,37 @@ 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(PP, this)); +} + static bool locationsInSameFile(const SourceManager &Sources, SourceLocation Loc1, SourceLocation Loc2) { return Loc1.isFileID() && Loc2.isFileID() && Sources.getFileID(Loc1) == Sources.getFileID(Loc2); } -static std::string getNamespaceComment(const NamespaceDecl *ND, - bool InsertLineBreak) { +std::string NamespaceCommentCheck::getNamespaceComment(const NamespaceDecl *ND, + bool InsertLineBreak) { std::string Fix = "// namespace"; - if (!ND->isAnonymousNamespace()) - Fix.append(" ").append(ND->getNameAsString()); + if (!ND->isAnonymousNamespace()) { + bool IsNamespaceMacroExpansion; + StringRef MacroDefinition; + std::tie(IsNamespaceMacroExpansion, MacroDefinition) = + isNamespaceMacroExpansion(ND->getName()); + + Fix.append(" ").append(IsNamespaceMacroExpansion ? MacroDefinition + : ND->getName()); + } if (InsertLineBreak) Fix.append("\n"); return Fix; } -static std::string getNamespaceComment(const std::string &NameSpaceName, - bool InsertLineBreak) { +std::string +NamespaceCommentCheck::getNamespaceComment(const std::string &NameSpaceName, + bool InsertLineBreak) { std::string Fix = "// namespace "; Fix.append(NameSpaceName); if (InsertLineBreak) @@ -65,6 +116,32 @@ static std::string 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 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("namespace"); const SourceManager &Sources = *Result.SourceManager; @@ -143,28 +220,48 @@ 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())) { + (((ND->getNameAsString() == NamespaceNameInComment) && + Anonymous.empty()) && + !IsNamespaceMacroExpansion)) { // 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; } + // 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())); - Message = - (llvm::Twine( - "%0 ends with a comment that refers to a wrong namespace '") + - NamespaceNameInComment + "'") - .str(); + + 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(); + } + } else if (Comment.startswith("//")) { // Assume that this is an unrecognized form of a namespace closing line // comment. Replace it. @@ -177,6 +274,16 @@ 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 712cd4662965e..bc5c11e7b71b9 100644 --- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h +++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h @@ -26,14 +26,29 @@ 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 + isNamespaceMacroExpansion(const StringRef NameSpaceName); llvm::Regex NamespaceCommentPattern; const unsigned ShortNamespaceLines; const unsigned SpacesBeforeComments; llvm::SmallVector Ends; + + // Store macros to verify that warning is not thrown when namespace name is a + // preprocessed define. + std::vector> 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 591c9dae5a74e..b4e79c97c0056 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_expansion' not terminated with -// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here +// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with +// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here } -// CHECK-FIXES: } // namespace macro_expansion +// CHECK-FIXES: } // namespace MACRO 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 new file mode 100644 index 0000000000000..a7d315693421d --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp @@ -0,0 +1,41 @@ +// 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