[clang-tools-extra] 665bfbb - Reland "[clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks""
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Fri May 20 12:12:56 PDT 2022
Author: Balazs Benics
Date: 2022-05-20T21:12:39+02:00
New Revision: 665bfbb98daa0e40a2e6cecfbbfd6949980d8f2f
URL: https://github.com/llvm/llvm-project/commit/665bfbb98daa0e40a2e6cecfbbfd6949980d8f2f
DIFF: https://github.com/llvm/llvm-project/commit/665bfbb98daa0e40a2e6cecfbbfd6949980d8f2f.diff
LOG: Reland "[clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks""
This partially reverts commit e8cae487022c2216182ae1ec24f248f287a614b7.
Changes since that commit:
- Use `SourceManager::isBeforeInTranslationUnit` instead of the fancy
decomposed decl logarithmic search.
- Add a test for including a system header containing a deprecated
include.
- Add `REQUIRES: system-linux` clause to the test.
Reviewed By: LegalizeAdulthood, whisperity
Differential Revision: https://reviews.llvm.org/D125209
Added:
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h
clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp
Modified:
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
index 0b44bdaafe23e..ee78747d397ff 100644
--- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
@@ -7,23 +7,27 @@
//===----------------------------------------------------------------------===//
#include "DeprecatedHeadersCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/PPCallbacks.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringSet.h"
+#include <algorithm>
#include <vector>
+using IncludeMarker =
+ clang::tidy::modernize::DeprecatedHeadersCheck::IncludeMarker;
namespace clang {
namespace tidy {
namespace modernize {
-
namespace {
+
class IncludeModernizePPCallbacks : public PPCallbacks {
public:
- explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check,
- LangOptions LangOpts);
+ explicit IncludeModernizePPCallbacks(
+ std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts);
void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
StringRef FileName, bool IsAngled,
@@ -33,22 +37,95 @@ class IncludeModernizePPCallbacks : public PPCallbacks {
SrcMgr::CharacteristicKind FileType) override;
private:
- ClangTidyCheck &Check;
+ std::vector<IncludeMarker> &IncludesToBeProcessed;
LangOptions LangOpts;
llvm::StringMap<std::string> CStyledHeaderToCxx;
llvm::StringSet<> DeleteHeaders;
};
+
+class ExternCRefutationVisitor
+ : public RecursiveASTVisitor<ExternCRefutationVisitor> {
+ std::vector<IncludeMarker> &IncludesToBeProcessed;
+ const SourceManager &SM;
+
+public:
+ ExternCRefutationVisitor(std::vector<IncludeMarker> &IncludesToBeProcessed,
+ SourceManager &SM)
+ : IncludesToBeProcessed(IncludesToBeProcessed), SM(SM) {}
+ bool shouldWalkTypesOfTypeLocs() const { return false; }
+ bool shouldVisitLambdaBody() const { return false; }
+
+ bool VisitLinkageSpecDecl(LinkageSpecDecl *LinkSpecDecl) const {
+ if (LinkSpecDecl->getLanguage() != LinkageSpecDecl::lang_c ||
+ !LinkSpecDecl->hasBraces())
+ return true;
+
+ auto ExternCBlockBegin = LinkSpecDecl->getBeginLoc();
+ auto ExternCBlockEnd = LinkSpecDecl->getEndLoc();
+ auto IsWrapped = [=, &SM = SM](const IncludeMarker &Marker) -> bool {
+ return SM.isBeforeInTranslationUnit(ExternCBlockBegin, Marker.DiagLoc) &&
+ SM.isBeforeInTranslationUnit(Marker.DiagLoc, ExternCBlockEnd);
+ };
+
+ llvm::erase_if(IncludesToBeProcessed, IsWrapped);
+ return true;
+ }
+};
} // namespace
+DeprecatedHeadersCheck::DeprecatedHeadersCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+
void DeprecatedHeadersCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
- PP->addPPCallbacks(
- ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts()));
+ PP->addPPCallbacks(std::make_unique<IncludeModernizePPCallbacks>(
+ IncludesToBeProcessed, getLangOpts()));
+}
+void DeprecatedHeadersCheck::registerMatchers(
+ ast_matchers::MatchFinder *Finder) {
+ // Even though the checker operates on a "preprocessor" level, we still need
+ // to act on a "TranslationUnit" to acquire the AST where we can walk each
+ // Decl and look for `extern "C"` blocks where we will suppress the report we
+ // collected during the preprocessing phase.
+ // The `onStartOfTranslationUnit()` won't suffice, since we need some handle
+ // to the `ASTContext`.
+ Finder->addMatcher(ast_matchers::translationUnitDecl().bind("TU"), this);
+}
+
+void DeprecatedHeadersCheck::onEndOfTranslationUnit() {
+ IncludesToBeProcessed.clear();
+}
+
+void DeprecatedHeadersCheck::check(
+ const ast_matchers::MatchFinder::MatchResult &Result) {
+ SourceManager &SM = Result.Context->getSourceManager();
+
+ // Suppress includes wrapped by `extern "C" { ... }` blocks.
+ ExternCRefutationVisitor Visitor(IncludesToBeProcessed, SM);
+ Visitor.TraverseAST(*Result.Context);
+
+ // Emit all the remaining reports.
+ for (const IncludeMarker &Marker : IncludesToBeProcessed) {
+ if (Marker.Replacement.empty()) {
+ diag(Marker.DiagLoc,
+ "including '%0' has no effect in C++; consider removing it")
+ << Marker.FileName
+ << FixItHint::CreateRemoval(Marker.ReplacementRange);
+ } else {
+ diag(Marker.DiagLoc, "inclusion of deprecated C++ header "
+ "'%0'; consider using '%1' instead")
+ << Marker.FileName << Marker.Replacement
+ << FixItHint::CreateReplacement(
+ Marker.ReplacementRange,
+ (llvm::Twine("<") + Marker.Replacement + ">").str());
+ }
+ }
}
-IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check,
- LangOptions LangOpts)
- : Check(Check), LangOpts(LangOpts) {
+IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(
+ std::vector<IncludeMarker> &IncludesToBeProcessed, LangOptions LangOpts)
+ : IncludesToBeProcessed(IncludesToBeProcessed), LangOpts(LangOpts) {
for (const auto &KeyValue :
std::vector<std::pair<llvm::StringRef, std::string>>(
{{"assert.h", "cassert"},
@@ -101,19 +178,15 @@ void IncludeModernizePPCallbacks::InclusionDirective(
// 1. Insert std prefix for every such symbol occurrence.
// 2. Insert `using namespace std;` to the beginning of TU.
// 3. Do nothing and let the user deal with the migration himself.
+ SourceLocation DiagLoc = FilenameRange.getBegin();
if (CStyledHeaderToCxx.count(FileName) != 0) {
- std::string Replacement =
- (llvm::Twine("<") + CStyledHeaderToCxx[FileName] + ">").str();
- Check.diag(FilenameRange.getBegin(), "inclusion of deprecated C++ header "
- "'%0'; consider using '%1' instead")
- << FileName << CStyledHeaderToCxx[FileName]
- << FixItHint::CreateReplacement(FilenameRange.getAsRange(),
- Replacement);
+ IncludesToBeProcessed.push_back(
+ IncludeMarker{CStyledHeaderToCxx[FileName], FileName,
+ FilenameRange.getAsRange(), DiagLoc});
} else if (DeleteHeaders.count(FileName) != 0) {
- Check.diag(FilenameRange.getBegin(),
- "including '%0' has no effect in C++; consider removing it")
- << FileName << FixItHint::CreateRemoval(
- SourceRange(HashLoc, FilenameRange.getEnd()));
+ IncludesToBeProcessed.push_back(
+ IncludeMarker{std::string{}, FileName,
+ SourceRange{HashLoc, FilenameRange.getEnd()}, DiagLoc});
}
}
diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h
index 113b0ca182e0f..2af626d42129b 100644
--- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h
@@ -34,13 +34,25 @@ namespace modernize {
/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-headers.html
class DeprecatedHeadersCheck : public ClangTidyCheck {
public:
- DeprecatedHeadersCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ DeprecatedHeadersCheck(StringRef Name, ClangTidyContext *Context);
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void onEndOfTranslationUnit() override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+ struct IncludeMarker {
+ std::string Replacement;
+ StringRef FileName;
+ SourceRange ReplacementRange;
+ SourceLocation DiagLoc;
+ };
+
+private:
+ std::vector<IncludeMarker> IncludesToBeProcessed;
};
} // namespace modernize
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 00124b0cad75d..806e5d10a5fdc 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,6 +170,11 @@ Changes in existing checks
<clang-tidy/checks/misc-redundant-expression>` involving assignments in
conditions. This fixes `Issue 35853 <https://github.com/llvm/llvm-project/issues/35853>`_.
+- Fixed a false positive in :doc:`modernize-deprecated-headers
+ <clang-tidy/checks/modernize-deprecated-headers>` involving including
+ C header files from C++ files wrapped by ``extern "C" { ... }`` blocks.
+ Such includes will be ignored by now.
+
- Improved :doc:`performance-inefficient-vector-operation
<clang-tidy/checks/performance-inefficient-vector-operation>` to work when
the vector is a member of a structure.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h
new file mode 100644
index 0000000000000..f5711e7692e70
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h
@@ -0,0 +1 @@
+#include <assert.h>
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h
new file mode 100644
index 0000000000000..f5711e7692e70
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h
@@ -0,0 +1 @@
+#include <assert.h>
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp
new file mode 100644
index 0000000000000..458c329de7743
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp
@@ -0,0 +1,66 @@
+
+// Copy the 'mylib.h' to a directory under the build directory. This is
+// required, since the relative order of the emitted diagnostics depends on the
+// absolute file paths which is sorted by clang-tidy prior emitting.
+//
+// RUN: mkdir -p %t/sys && mkdir -p %t/usr \
+// RUN: && cp %S/Inputs/modernize-deprecated-headers/mysystemlib.h %t/sys/mysystemlib.h \
+// RUN: && cp %S/Inputs/modernize-deprecated-headers/mylib.h %t/usr/mylib.h
+
+// RUN: %check_clang_tidy -std=c++11 %s modernize-deprecated-headers %t \
+// RUN: --header-filter='.*' --system-headers \
+// RUN: -- -I %t/usr -isystem %t/sys -isystem %S/Inputs/modernize-deprecated-headers
+
+// REQUIRES: system-linux
+
+#define EXTERN_C extern "C"
+
+extern "C++" {
+// We should still have the warnings here.
+#include <stdbool.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
+}
+
+#include <assert.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
+
+#include <stdbool.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers]
+
+#include <mysystemlib.h> // FIXME: We should have no warning into system headers.
+// CHECK-MESSAGES: mysystemlib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
+
+#include <mylib.h>
+// CHECK-MESSAGES: mylib.h:1:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
+
+namespace wrapping {
+extern "C" {
+#include <assert.h> // no-warning
+#include <mylib.h> // no-warning
+#include <stdbool.h> // no-warning
+}
+} // namespace wrapping
+
+extern "C" {
+namespace wrapped {
+#include <assert.h> // no-warning
+#include <mylib.h> // no-warning
+#include <stdbool.h> // no-warning
+} // namespace wrapped
+}
+
+namespace wrapping {
+extern "C" {
+namespace wrapped {
+#include <assert.h> // no-warning
+#include <mylib.h> // no-warning
+#include <stdbool.h> // no-warning
+} // namespace wrapped
+}
+} // namespace wrapping
+
+EXTERN_C {
+#include <assert.h> // no-warning
+#include <mylib.h> // no-warning
+#include <stdbool.h> // no-warning
+}
More information about the cfe-commits
mailing list