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 @@ -15,6 +15,17 @@ namespace tidy { namespace modernize { +namespace detail { +class IncludeModernizePPCallbacks; +class ExternCRefutationVisitor; +struct IncludeMarker { + std::string Replacement; + StringRef FileName; + SourceRange ReplacementRange; + SourceLocation DiagLoc; +}; +} // namespace detail + /// This check replaces deprecated C library headers with their C++ STL /// alternatives. /// @@ -34,13 +45,20 @@ /// 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; + +private: + friend class detail::IncludeModernizePPCallbacks; + friend class detail::ExternCRefutationVisitor; + 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,22 +7,23 @@ //===----------------------------------------------------------------------===// #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 namespace clang { namespace tidy { namespace modernize { - -namespace { +namespace detail { class IncludeModernizePPCallbacks : public PPCallbacks { public: - explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check, + explicit IncludeModernizePPCallbacks(DeprecatedHeadersCheck &Check, LangOptions LangOpts); void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, @@ -33,21 +34,94 @@ SrcMgr::CharacteristicKind FileType) override; private: - ClangTidyCheck &Check; + DeprecatedHeadersCheck &Check; LangOptions LangOpts; llvm::StringMap CStyledHeaderToCxx; llvm::StringSet<> DeleteHeaders; }; -} // namespace + +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 detail + +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( + *this, 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. + detail::ExternCRefutationVisitor Visitor(IncludesToBeProcessed, SM); + Visitor.TraverseAST(*Result.Context); + + // Emit all the remaining reports. + for (const detail::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) +detail::IncludeModernizePPCallbacks::IncludeModernizePPCallbacks( + DeprecatedHeadersCheck &Check, LangOptions LangOpts) : Check(Check), LangOpts(LangOpts) { for (const auto &KeyValue : std::vector>( @@ -89,7 +163,7 @@ } } -void IncludeModernizePPCallbacks::InclusionDirective( +void detail::IncludeModernizePPCallbacks::InclusionDirective( SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, CharSourceRange FilenameRange, Optional File, StringRef SearchPath, StringRef RelativePath, const Module *Imported, @@ -101,19 +175,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); + Check.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())); + Check.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 +}