Skip to content

Implement a cmp specialization for ranges #60337

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

Open
LilithHafner wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement a cmp specialization for ranges #60337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ee77b2
Implement a cmp specializaiton for ranges
LilithHafner Dec 7, 2025
80dd45a
Add tests (gen AI helpted populate const EXAMPLE_RANGES)
LilithHafner Dec 7, 2025
3b6d103
Add example from discussion to tests
LilithHafner Dec 7, 2025
3f344d3
Reduce some explicit/syntactic complexity in exchange for a larger (b…
LilithHafner Dec 7, 2025
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
10 changes: 10 additions & 0 deletions base/range.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,16 @@ function ==(r::AbstractRange, s::AbstractRange)
return true
end

function cmp(r1::AbstractRange{T}, r2::AbstractRange{T}) where {T}
firstindex(r1) == firstindex(r2) || return cmp(firstindex(r1), firstindex(r2))
(isempty(r1) || isempty(r2)) && return cmp(isempty(r2), isempty(r1))
first(r1) != first(r2) && return cmp(first(r1), first(r2))
# Assume that ranges are monotonic and use the last shared element as a high precision proxy for step.
x1, x2 = last(zip(r1, r2))
x1 != x2 && return cmp(x1, x2)
cmp(length(r1), length(r2))
Comment on lines +1175 to +1178
Copy link
Member

@mbauman mbauman Dec 8, 2025

Choose a reason for hiding this comment

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

last(zip(r1, r2)) is still the abstract iterator's O(n) isn't it? And if we're doing an O(n) thing, we might as well compare everything, no? The goal here should be to detect the "fast" outs that are definitively correct, falling back to O(n) iteration if we can't do that.

The worst case is where step is identical, but, yeah, it's tricky to rely on step(r). Perhaps this should just be done for OrdinalRanges.

Copy link
Member

@adienes adienes Dec 8, 2025

Choose a reason for hiding this comment

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

last(::Zip) is actually O(1) in most cases but I agree it will be challenging for any generic implementation to work for all ranges

end

intersect(r::OneTo, s::OneTo) = OneTo(min(r.stop,s.stop))
union(r::OneTo, s::OneTo) = OneTo(max(r.stop,s.stop))

Expand Down
27 changes: 27 additions & 0 deletions test/ranges.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2831,3 +2831,30 @@ end
@test StepRange(r) == r
@test StepRange(r) isa StepRange{Date,Day}
end

const EXAMPLE_RANGES = AbstractRange[
1:10,
1:5,
1:0,
1:1,
3:2,
3:0,
10:-1:1,
1:2:10,
10:-2:1,
LinRange(1.0, 10.0, 10),
LinRange(10.0, 1.0, 10),
1e10:1.99:(1e10 + 2),
1e10:(1.99+eps()):(1e10 + 2),
StepRangeLen(1, 2, 5),
StepRangeLen(10, -2, 5),
UInt8(1):UInt8(10),
UInt8(10):-UInt8(1):UInt8(1),
'a':'z',
]

@testset "cmp(::AbstractRange, ::AbstractRange)" begin
for a in EXAMPLE_RANGES, b in EXAMPLE_RANGES
@test try cmp(a, b) catch e; e end == try cmp(collect(a), collect(b)) catch e; e end
Copy link
Contributor

@Seelengrab Seelengrab Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
@test try cmp(a, b) catch e; e end == try cmp(collect(a), collect(b)) catch e; e end
@test cmp(a, b) == cmp(collect(a), collect(b))

Isn't exception handling done by @test already?

Copy link
Contributor

@Seelengrab Seelengrab Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or is this to compare that the errors are the same..? If so, IMO that should be done in a test explicit for that case, rather than mixing it into the general one. Alternatively we don't check this at all, since just comparing the thrown object doesn't mean that it's the same error - the location it was thrown from can be different too.

end
end