[clang-tools-extra] 4736d63 - Fix llvm-namespace-comment for macro expansions
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Sat Nov 23 10:09:02 PST 2019
Author: Marcin Twardak
Date: 2019-11-23T13:08:14-05:00
New Revision: 4736d63f752f8d13f4c6a9afd558565c32119718
URL: https://github.com/llvm/llvm-project/commit/4736d63f752f8d13f4c6a9afd558565c32119718
DIFF: https://github.com/llvm/llvm-project/commit/4736d63f752f8d13f4c6a9afd558565c32119718.diff
LOG: Fix llvm-namespace-comment for macro expansions
If a namespace is a macro name, it should be allowed to close the
namespace with the same name.
Added:
clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
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:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
index eb3d7c505b83..a2a56241e8ab 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<NamespaceCommentPPCallbacks>(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<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;
@@ -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
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()));
- 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 712cd4662965..bc5c11e7b71b 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<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 591c9dae5a74..b4e79c97c005 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 000000000000..a7d315693421
--- /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
More information about the cfe-commits
mailing list