twardakm updated this revision to Diff 230113.
twardakm added a comment.
Apply Aaron's comments
- Fix missing namespace to macro definition instead of extension
- Print macro definition also in warning message
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69855/new/
https://reviews.llvm.org/D69855
Files:
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
clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
@@ -25,10 +25,10 @@
// 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 {
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h
@@ -26,14 +26,29 @@
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
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -19,6 +19,44 @@
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,39 @@
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());
+
+ if (!IsNamespaceMacroExpansion)
+ Fix.append(" ").append(ND->getNameAsString());
+ else
+ Fix.append(" ").append(MacroDefinition);
+ }
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 +118,32 @@
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 = std::find_if(std::begin(Macros), std::end(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 +222,49 @@
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 +277,17 @@
// 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"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
