From 6902e520d60b3fd1ff14401db5fafcbea1ff9d8a Mon Sep 17 00:00:00 2001 From: Nathan James Date: Thu, 25 Jul 2024 16:25:37 +0100 Subject: [PATCH] [clang-tidy] Fix crash in modernize-use-ranges (#100427) Summary: Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues Fixes #100406 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250581 --- .../clang-tidy/utils/UseRangesCheck.cpp | 64 +++++++++---------- .../clang-tidy/utils/UseRangesCheck.h | 2 +- .../modernize/Inputs/use-ranges/fake_std.h | 17 +++-- 3 files changed, 43 insertions(+), 40 deletions(-) diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp index e2daa5010e2aeb..aba4d17ccd035e 100644 --- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp @@ -39,12 +39,6 @@ static constexpr const char ArgName[] = "ArgName"; namespace clang::tidy::utils { -static bool operator==(const UseRangesCheck::Indexes &L, - const UseRangesCheck::Indexes &R) { - return std::tie(L.BeginArg, L.EndArg, L.ReplaceArg) == - std::tie(R.BeginArg, R.EndArg, R.ReplaceArg); -} - static std::string getFullPrefix(ArrayRef Signature) { std::string Output; llvm::raw_string_ostream OS(Output); @@ -54,15 +48,6 @@ static std::string getFullPrefix(ArrayRef Signature) { return Output; } -static llvm::hash_code hash_value(const UseRangesCheck::Indexes &Indexes) { - return llvm::hash_combine(Indexes.BeginArg, Indexes.EndArg, - Indexes.ReplaceArg); -} - -static llvm::hash_code hash_value(const UseRangesCheck::Signature &Sig) { - return llvm::hash_combine_range(Sig.begin(), Sig.end()); -} - namespace { AST_MATCHER(Expr, hasSideEffects) { @@ -123,24 +108,26 @@ makeMatcherPair(StringRef State, const UseRangesCheck::Indexes &Indexes, } void UseRangesCheck::registerMatchers(MatchFinder *Finder) { - Replaces = getReplacerMap(); + auto Replaces = getReplacerMap(); ReverseDescriptor = getReverseDescriptor(); auto BeginEndNames = getFreeBeginEndMethods(); llvm::SmallVector BeginNames{ llvm::make_first_range(BeginEndNames)}; llvm::SmallVector EndNames{ llvm::make_second_range(BeginEndNames)}; - llvm::DenseSet> Seen; + Replacers.clear(); + llvm::DenseSet SeenRepl; for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) { - const ArrayRef &Signatures = - I->getValue()->getReplacementSignatures(); - if (!Seen.insert(Signatures).second) + auto Replacer = I->getValue(); + if (!SeenRepl.insert(Replacer.get()).second) continue; - assert(!Signatures.empty() && - llvm::all_of(Signatures, [](auto Index) { return !Index.empty(); })); + Replacers.push_back(Replacer); + assert(!Replacer->getReplacementSignatures().empty() && + llvm::all_of(Replacer->getReplacementSignatures(), + [](auto Index) { return !Index.empty(); })); std::vector Names(1, I->getKey()); for (auto J = std::next(I); J != E; ++J) - if (J->getValue()->getReplacementSignatures() == Signatures) + if (J->getValue() == Replacer) Names.push_back(J->getKey()); std::vector TotalMatchers; @@ -148,7 +135,7 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) { // signatures in order of length(longest to shortest). This way any // signature that is a subset of another signature will be matched after the // other. - SmallVector SigVec(Signatures); + SmallVector SigVec(Replacer->getReplacementSignatures()); llvm::sort(SigVec, [](auto &L, auto &R) { return R.size() < L.size(); }); for (const auto &Signature : SigVec) { std::vector Matchers; @@ -163,7 +150,8 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) { } Finder->addMatcher( callExpr( - callee(functionDecl(hasAnyName(std::move(Names))).bind(FuncDecl)), + callee(functionDecl(hasAnyName(std::move(Names))) + .bind((FuncDecl + Twine(Replacers.size() - 1).str()))), ast_matchers::internal::DynTypedMatcher::constructVariadic( ast_matchers::internal::DynTypedMatcher::VO_AnyOf, ASTNodeKind::getFromNodeKind(), @@ -205,21 +193,33 @@ static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call, } void UseRangesCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Function = Result.Nodes.getNodeAs(FuncDecl); - std::string Qualified = "::" + Function->getQualifiedNameAsString(); - auto Iter = Replaces.find(Qualified); - assert(Iter != Replaces.end()); + Replacer *Replacer = nullptr; + const FunctionDecl *Function = nullptr; + for (auto [Node, Value] : Result.Nodes.getMap()) { + StringRef NodeStr(Node); + if (!NodeStr.consume_front(FuncDecl)) + continue; + Function = Value.get(); + size_t Index; + if (NodeStr.getAsInteger(10, Index)) { + llvm_unreachable("Unable to extract replacer index"); + } + assert(Index < Replacers.size()); + Replacer = Replacers[Index].get(); + break; + } + assert(Replacer && Function); SmallString<64> Buffer; - for (const Signature &Sig : Iter->getValue()->getReplacementSignatures()) { + for (const Signature &Sig : Replacer->getReplacementSignatures()) { Buffer.assign({BoundCall, getFullPrefix(Sig)}); const auto *Call = Result.Nodes.getNodeAs(Buffer); if (!Call) continue; auto Diag = createDiag(*Call); - if (auto ReplaceName = Iter->getValue()->getReplaceName(*Function)) + if (auto ReplaceName = Replacer->getReplaceName(*Function)) Diag << FixItHint::CreateReplacement(Call->getCallee()->getSourceRange(), *ReplaceName); - if (auto Include = Iter->getValue()->getHeaderInclusion(*Function)) + if (auto Include = Replacer->getHeaderInclusion(*Function)) Diag << Inserter.createIncludeInsertion( Result.SourceManager->getFileID(Call->getBeginLoc()), *Include); llvm::SmallVector ToRemove; diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h index 927e9694b0ec7c..3a454bcf0cf07a 100644 --- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h +++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h @@ -85,7 +85,7 @@ class UseRangesCheck : public ClangTidyCheck { std::optional getCheckTraversalKind() const override; private: - ReplacerMap Replaces; + std::vector> Replacers; std::optional ReverseDescriptor; IncludeInserter Inserter; }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h index 6596511c7a38bb..69ac9954f4afa9 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h @@ -7,8 +7,8 @@ template class vector { public: using iterator = T *; using const_iterator = const T *; - using reverse_iterator = T*; - using reverse_const_iterator = const T*; + using reverse_iterator = T *; + using reverse_const_iterator = const T *; constexpr const_iterator begin() const; constexpr const_iterator end() const; @@ -72,8 +72,8 @@ template constexpr auto crend(const Container &Cont) { return Cont.crend(); } // Find -template< class InputIt, class T > -InputIt find( InputIt first, InputIt last, const T& value ); +template +InputIt find(InputIt first, InputIt last, const T &value); // Reverse template void reverse(Iter begin, Iter end); @@ -82,6 +82,7 @@ template void reverse(Iter begin, Iter end); template bool includes(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2); +inline namespace _V1 { // IsPermutation template bool is_permutation(ForwardIt1 first1, ForwardIt1 last1, ForwardIt2 first2); @@ -97,9 +98,10 @@ template bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2); template -bool equal(InputIt1 first1, InputIt1 last1, - InputIt2 first2, InputIt2 last2, BinaryPred p) { - // Need a definition to suppress undefined_internal_type when invoked with lambda +bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2, + BinaryPred p) { + // Need a definition to suppress undefined_internal_type when invoked with + // lambda return true; } @@ -108,6 +110,7 @@ void iota(ForwardIt first, ForwardIt last, T value); template ForwardIt rotate(ForwardIt first, ForwardIt middle, ForwardIt last); +} // namespace _V1 } // namespace std