diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h --- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h @@ -34,13 +34,25 @@ /// 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 IncludesToBeProcessed; }; } // namespace modernize diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp --- 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 #include +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 &IncludesToBeProcessed, LangOptions LangOpts); void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, @@ -33,22 +37,95 @@ SrcMgr::CharacteristicKind FileType) override; private: - ClangTidyCheck &Check; + std::vector &IncludesToBeProcessed; LangOptions LangOpts; llvm::StringMap CStyledHeaderToCxx; llvm::StringSet<> DeleteHeaders; }; + +class ExternCRefutationVisitor + : public RecursiveASTVisitor { + std::vector &IncludesToBeProcessed; + const SourceManager &SM; + +public: + ExternCRefutationVisitor(std::vector &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(*this, getLangOpts())); + PP->addPPCallbacks(std::make_unique( + 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 &IncludesToBeProcessed, LangOptions LangOpts) + : IncludesToBeProcessed(IncludesToBeProcessed), LangOpts(LangOpts) { for (const auto &KeyValue : std::vector>( {{"assert.h", "cassert"}, @@ -101,19 +178,15 @@ // 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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -170,6 +170,11 @@ ` involving assignments in conditions. This fixes `Issue 35853 `_. +- Fixed a false positive in :doc:`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 ` 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 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h @@ -0,0 +1 @@ +#include 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 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mysystemlib.h @@ -0,0 +1 @@ +#include 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 --- /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 +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers] +} + +#include +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers] + +#include +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [modernize-deprecated-headers] + +#include // 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 +// 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 // no-warning +#include // no-warning +#include // no-warning +} +} // namespace wrapping + +extern "C" { +namespace wrapped { +#include // no-warning +#include // no-warning +#include // no-warning +} // namespace wrapped +} + +namespace wrapping { +extern "C" { +namespace wrapped { +#include // no-warning +#include // no-warning +#include // no-warning +} // namespace wrapped +} +} // namespace wrapping + +EXTERN_C { +#include // no-warning +#include // no-warning +#include // no-warning +}