Author: Balazs Benics Date: 2022-05-13T17:07:58+02:00 New Revision: e8cae487022c2216182ae1ec24f248f287a614b7
URL: https://github.com/llvm/llvm-project/commit/e8cae487022c2216182ae1ec24f248f287a614b7 DIFF: https://github.com/llvm/llvm-project/commit/e8cae487022c2216182ae1ec24f248f287a614b7.diff LOG: Revert "[clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks" This reverts commit 7e3ea55da88a9d7feaa22f29d51f89fd0152a189. Looks like this breaks tests: http://45.33.8.238/linux/76033/step_8.txt Added: Modified: clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h clang-tools-extra/docs/ReleaseNotes.rst Removed: 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 ################################################################################ diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp index dd991221faab..0b44bdaafe23 100644 --- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp @@ -7,37 +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 <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(DeprecatedHeadersCheck &Check, - LangOptions LangOpts, - const SourceManager &SM); + explicit IncludeModernizePPCallbacks(ClangTidyCheck &Check, + LangOptions LangOpts); void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, @@ -47,98 +33,22 @@ class IncludeModernizePPCallbacks : public PPCallbacks { SrcMgr::CharacteristicKind FileType) override; private: - DeprecatedHeadersCheck &Check; + ClangTidyCheck &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 detail +} // namespace void DeprecatedHeadersCheck::registerPPCallbacks( const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { - 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()); - } - } + PP->addPPCallbacks( + ::std::make_unique<IncludeModernizePPCallbacks>(*this, getLangOpts())); } -detail::IncludeModernizePPCallbacks::IncludeModernizePPCallbacks( - DeprecatedHeadersCheck &Check, LangOptions LangOpts, - const SourceManager &SM) - : Check(Check), LangOpts(LangOpts), SM(SM) { +IncludeModernizePPCallbacks::IncludeModernizePPCallbacks(ClangTidyCheck &Check, + LangOptions LangOpts) + : Check(Check), LangOpts(LangOpts) { for (const auto &KeyValue : std::vector<std::pair<llvm::StringRef, std::string>>( {{"assert.h", "cassert"}, @@ -179,7 +89,7 @@ detail::IncludeModernizePPCallbacks::IncludeModernizePPCallbacks( } } -void detail::IncludeModernizePPCallbacks::InclusionDirective( +void IncludeModernizePPCallbacks::InclusionDirective( SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName, bool IsAngled, CharSourceRange FilenameRange, Optional<FileEntryRef> File, StringRef SearchPath, StringRef RelativePath, const Module *Imported, @@ -191,16 +101,19 @@ void detail::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) { - Check.IncludesToBeProcessed.push_back( - IncludeMarker{CStyledHeaderToCxx[FileName], FileName, - FilenameRange.getAsRange(), DiagLoc}); + 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); } else if (DeleteHeaders.count(FileName) != 0) { - Check.IncludesToBeProcessed.push_back( - IncludeMarker{std::string{}, FileName, - SourceRange{HashLoc, FilenameRange.getEnd()}, DiagLoc}); + Check.diag(FilenameRange.getBegin(), + "including '%0' has no effect in C++; consider removing it") + << FileName << FixItHint::CreateRemoval( + SourceRange(HashLoc, FilenameRange.getEnd())); } } diff --git a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h index 1b76ca2ba2bb..113b0ca182e0 100644 --- a/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h @@ -15,22 +15,6 @@ 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. /// @@ -57,14 +41,6 @@ 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 806e5d10a5fd..00124b0cad75 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -170,11 +170,6 @@ 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 deleted file mode 100644 index e6adb8fd98e8..000000000000 --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-deprecated-headers/mylib.h +++ /dev/null @@ -1 +0,0 @@ -#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 deleted file mode 100644 index 344f7ac5a289..000000000000 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp +++ /dev/null @@ -1,53 +0,0 @@ -// 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