Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash in modernize-use-ranges #100427

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Fix crash in modernize-use-ranges #100427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 32 additions & 32 deletions clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<UseRangesCheck::Indexes> Signature) {
std::string Output;
llvm::raw_string_ostream OS(Output);
Expand All @@ -54,15 +48,6 @@ static std::string getFullPrefix(ArrayRef<UseRangesCheck::Indexes> 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) {
Expand Down Expand Up @@ -123,32 +108,34 @@ makeMatcherPair(StringRef State, const UseRangesCheck::Indexes &Indexes,
}

void UseRangesCheck::registerMatchers(MatchFinder *Finder) {
Replaces = getReplacerMap();
auto Replaces = getReplacerMap();
ReverseDescriptor = getReverseDescriptor();
auto BeginEndNames = getFreeBeginEndMethods();
llvm::SmallVector<StringRef, 4> BeginNames{
llvm::make_first_range(BeginEndNames)};
llvm::SmallVector<StringRef, 4> EndNames{
llvm::make_second_range(BeginEndNames)};
llvm::DenseSet<ArrayRef<Signature>> Seen;
Replacers.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: that Replaces vs Replacers is confusing, consider changing a name of one later on main.

llvm::DenseSet<Replacer *> SeenRepl;
for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) {
const ArrayRef<Signature> &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<StringRef> 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<ast_matchers::internal::DynTypedMatcher> TotalMatchers;
// As we match on the first matched signature, we need to sort the
// 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<Signature> SigVec(Signatures);
SmallVector<Signature> SigVec(Replacer->getReplacementSignatures());
llvm::sort(SigVec, [](auto &L, auto &R) { return R.size() < L.size(); });
for (const auto &Signature : SigVec) {
std::vector<ast_matchers::internal::DynTypedMatcher> Matchers;
Expand All @@ -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<CallExpr>(),
Expand Down Expand Up @@ -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<FunctionDecl>(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<FunctionDecl>();
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<CallExpr>(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<unsigned, 3> ToRemove;
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clang-tidy/utils/UseRangesCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class UseRangesCheck : public ClangTidyCheck {
std::optional<TraversalKind> getCheckTraversalKind() const override;

private:
ReplacerMap Replaces;
std::vector<llvm::IntrusiveRefCntPtr<Replacer>> Replacers;
std::optional<ReverseIteratorDescriptor> ReverseDescriptor;
IncludeInserter Inserter;
};
Expand Down
17 changes: 10 additions & 7 deletions clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ template <typename T> 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;
Expand Down Expand Up @@ -72,8 +72,8 @@ template <typename Container> 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 <class InputIt, class T>
InputIt find(InputIt first, InputIt last, const T &value);

// Reverse
template <typename Iter> void reverse(Iter begin, Iter end);
Expand All @@ -82,6 +82,7 @@ template <typename Iter> void reverse(Iter begin, Iter end);
template <class InputIt1, class InputIt2>
bool includes(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2);

inline namespace _V1 {
// IsPermutation
template <class ForwardIt1, class ForwardIt2>
bool is_permutation(ForwardIt1 first1, ForwardIt1 last1, ForwardIt2 first2);
Expand All @@ -97,9 +98,10 @@ template <class InputIt1, class InputIt2>
bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2);

template <class InputIt1, class InputIt2, class BinaryPred>
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;
}

Expand All @@ -108,6 +110,7 @@ void iota(ForwardIt first, ForwardIt last, T value);

template <class ForwardIt>
ForwardIt rotate(ForwardIt first, ForwardIt middle, ForwardIt last);
} // namespace _V1

} // namespace std

Expand Down
Loading