Author: Balazs Benics Date: 2022-05-13T16:54:13+02:00 New Revision: 7e3ea55da88a9d7feaa22f29d51f89fd0152a189
URL: https://github.com/llvm/llvm-project/commit/7e3ea55da88a9d7feaa22f29d51f89fd0152a189 DIFF: https://github.com/llvm/llvm-project/commit/7e3ea55da88a9d7feaa22f29d51f89fd0152a189.diff LOG: [clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks The check should not report includes wrapped by `extern "C" { ... }` blocks, such as: ```lang=C++ #ifdef __cplusplus extern "C" { #endif #include "assert.h" #ifdef __cplusplus } #endif ``` This pattern comes up sometimes in header files designed to be consumed by both C and C++ source files. The check now reports false reports when the header file is consumed by a C++ translation unit. In this change, I'm not emitting the reports immediately from the `PPCallback`, rather aggregating them for further processing. After all preprocessing is done, the matcher will be called on the `TranslationUnitDecl`, ensuring that the check callback is called only once. Within that callback, I'm recursively visiting each decls, looking for `LinkageSpecDecls` which represent the `extern "C"` specifier. After this, I'm dropping all the reports coming from inside of it. After the visitation is done, I'm emitting the reports I'm left with. For performance reasons, I'm sorting the `IncludeMarkers` by their corresponding locations. This makes the scan `O(log(N)` when looking up the `IncludeMarkers` affected by the given `extern "C"` block. For this, I'm using `lower_bound()` and `upper_bound()`. Reviewed By: 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/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 0b44bdaafe23..dd991221faab 100644 --- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp @@ -7,23 +7,37 @@ //===----------------------------------------------------------------------===// #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> namespace clang { namespace tidy { namespace modernize { +namespace detail { +bool operator<(const IncludeMarker &LHS, const IncludeMarker &RHS) { + return LHS.DecomposedDiagLoc < RHS.DecomposedDiagLoc; +} +bool operator<(const IncludeMarker &LHS, + const std::pair<FileID, unsigned> &RHS) { + return LHS.DecomposedDiagLoc < RHS; +} +bool operator<(const std::pair<FileID, unsigned> &LHS, + const IncludeMarker &RHS) { + return LHS < RHS.DecomposedDiagLoc; +} -namespace { class IncludeModernizePPCallbacks : public PPCallbacks { public: - explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check, - LangOptions LangOpts); + explicit IncludeModernizePPCallbacks(DeprecatedHeadersCheck &Check, + LangOptions LangOpts, + const SourceManager &SM); void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, @@ -33,22 +47,98 @@ class IncludeModernizePPCallbacks : public PPCallbacks { SrcMgr::CharacteristicKind FileType) override; private: - ClangTidyCheck &Check; + DeprecatedHeadersCheck &Check; LangOptions LangOpts; llvm::StringMap<std::string> CStyledHeaderToCxx; llvm::StringSet<> DeleteHeaders; + const SourceManager &SM; +}; + +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 = + SM.getDecomposedExpansionLoc(LinkSpecDecl->getBeginLoc()); + auto ExternCBlockEnd = + SM.getDecomposedExpansionLoc(LinkSpecDecl->getEndLoc()); + + auto Begin = IncludesToBeProcessed.begin(); + auto End = IncludesToBeProcessed.end(); + auto Low = std::lower_bound(Begin, End, ExternCBlockBegin); + auto Up = std::upper_bound(Low, End, ExternCBlockEnd); + IncludesToBeProcessed.erase(Low, Up); + return true; + } }; -} // namespace +} // namespace detail void DeprecatedHeadersCheck::registerPPCallbacks( const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { - PP->addPPCallbacks( - ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts())); + PP->addPPCallbacks(::std::make_unique<detail::IncludeModernizePPCallbacks>( + *this, getLangOpts(), PP->getSourceManager())); +} +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(); + using detail::IncludeMarker; + + llvm::sort(IncludesToBeProcessed); + + // Suppress includes wrapped by `extern "C" { ... }` blocks. + detail::ExternCRefutationVisitor Visitor(IncludesToBeProcessed, SM); + Visitor.TraverseAST(*Result.Context); + + // Emit all the remaining reports. + for (const IncludeMarker &Entry : IncludesToBeProcessed) { + SourceLocation DiagLoc = SM.getComposedLoc(Entry.DecomposedDiagLoc.first, + Entry.DecomposedDiagLoc.second); + if (Entry.Replacement.empty()) { + diag(DiagLoc, "including '%0' has no effect in C++; consider removing it") + << Entry.FileName << FixItHint::CreateRemoval(Entry.ReplacementRange); + } else { + diag(DiagLoc, "inclusion of deprecated C++ header " + "'%0'; consider using '%1' instead") + << Entry.FileName << Entry.Replacement + << FixItHint::CreateReplacement( + Entry.ReplacementRange, + (llvm::Twine("<") + Entry.Replacement + ">").str()); + } + } } -IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check, - LangOptions LangOpts) - : Check(Check), LangOpts(LangOpts) { +detail::IncludeModernizePPCallbacks::IncludeModernizePPCallbacks( + DeprecatedHeadersCheck &Check, LangOptions LangOpts, + const SourceManager &SM) + : Check(Check), LangOpts(LangOpts), SM(SM) { for (const auto &KeyValue : std::vector<std::pair<llvm::StringRef, std::string>>( {{"assert.h", "cassert"}, @@ -89,7 +179,7 @@ IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check, } } -void IncludeModernizePPCallbacks::InclusionDirective( +void detail::IncludeModernizePPCallbacks::InclusionDirective( SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, CharSourceRange FilenameRange, Optional<FileEntryRef> File, StringRef SearchPath, StringRef RelativePath, const Module *Imported, @@ -101,19 +191,16 @@ 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. + std::pair<FileID, unsigned> DiagLoc = + SM.getDecomposedExpansionLoc(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/clang-tidy/modernize/DeprecatedHeadersCheck.h b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h index 113b0ca182e0..1b76ca2ba2bb 100644 --- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h @@ -15,6 +15,22 @@ namespace clang { namespace tidy { namespace modernize { +namespace detail { +class IncludeModernizePPCallbacks; +class ExternCRefutationVisitor; +struct IncludeMarker { + std::string Replacement; + StringRef FileName; + SourceRange ReplacementRange; + std::pair<FileID, unsigned> DecomposedDiagLoc; +}; +bool operator<(const IncludeMarker &LHS, const IncludeMarker &RHS); +bool operator<(const IncludeMarker &LHS, + const std::pair<FileID, unsigned> &RHS); +bool operator<(const std::pair<FileID, unsigned> &LHS, + const IncludeMarker &RHS); +} // namespace detail + /// This check replaces deprecated C library headers with their C++ STL /// alternatives. /// @@ -41,6 +57,14 @@ class DeprecatedHeadersCheck : public ClangTidyCheck { } 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<detail::IncludeMarker> IncludesToBeProcessed; }; } // namespace modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 00124b0cad75..806e5d10a5fd 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 000000000000..e6adb8fd98e8 --- /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/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 000000000000..344f7ac5a289 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp @@ -0,0 +1,53 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s modernize-deprecated-headers %t \ +// RUN: -- -header-filter='.*' -system-headers \ +// RUN: -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers + +#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] +// CHECK-FIXES: {{^}}#include <cassert>{{$}} + +#include <stdbool.h> +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: including 'stdbool.h' has no effect in C++; consider removing it [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 +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
