-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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.
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
base: master
Are you sure you want to change the base?
Conversation
…ut equivalent and fully inlined) call graph
|
|
||
| @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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @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?
There was a problem hiding this comment.
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.
|
here is another good stressor I think forcing the fast path to have the same concrete types will resolve this type of edge case |
| # 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
cmpspecialization for rangesconst EXAMPLE_RANGES)Fixes #60334