[clang-tools-extra] c375dc2 - Revert "Fix llvm-namespace-comment for macro expansions"
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 3 11:31:54 PST 2019
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
More information about the cfe-commits
mailing list