phobos
phobos copied to clipboard
dlang
Fix Issue 10706 setops functions need to require SortedRanges
https://issues.dlang.org/show_bug.cgi?id=10706
- largestPartialIntersection
- largestPartialIntersectionWeighted
- multiwayMerge
- multiwayUnion
- setDifference
- setIntersection
- setSymmetricDifference
Now all require SortedRanges to be passed, as that is what they expect implicitly anyway.
A new public symbol isSortedRange was added to std.range.
Thanks for your pull request, @burner!
Bugzilla references
| Auto-close | Bugzilla | Severity | Description |
|---|---|---|---|
| ✓ | 10706 | enhancement | Functions that require a sorted range to take a SortedRange? |
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + phobos#6795"
LGTM, given the possible fixing breakage that this will incur, this needs a changelog.
I will do the suggested change, but SortedRange will check for sortedness in debug. So this is still a breaking change.
But from
Assuming for the sake of argument that this should go ahead
it sounds like an opinion has already been formed regarding this PR. If this would be my first PR it would be my last. It's the tone that makes the music.
I'm sorry it came across that way. I think the red X makes the tone seem more negative than intended.
@n8sh I didn't notice the red X until you just mentioned it.
Anyway, what is the way forward with this.
Anyway, what is the way forward with this.
You said you intend to do the suggested change, so when that is done this should be ready to go ahead.
I already added the overloads.
But I can't mark them as deprecated as phobos will fail to build.
I already added the overloads.
Could you please point me to them.
But I can't mark them as deprecated as phobos will fail to build.
Bummer.
But I can't mark them as deprecated as phobos will fail to build.
Well, all tests that use them need to be marked as deprecated too then (or use assumeSorted or similar).
All tests that rely on the non SortedRange input have been marked deprecated. I can't mark the non SortedRange version of the functions deprecated as they call each other.
I'm inclined to add a comment saying the will go away at some point in the future.
I can't mark the non SortedRange version of the functions deprecated as they call each other.
Can't each non-SortedRange function just call the SortedRange equivalent?
I hope to wrap sub calls in assumeSorted, by that doesn't work as I would have to reconstruct RangeOfRanges which could be everything. I think this is as good as it gets right now.
added a red comment in the docs to state that the usage of non SortedRange ranges is deprecated
I can't mark the non SortedRange version of the functions deprecated as they call each other.
I think they can all be marked deprecated without a problem except for multiwayMerge (EDIT: and largestPartialIntersectionWeighted). I'll push that change to this PR.
@burner I know this is old, but any chance of applying @andralex 's feedback and then tackle another round of review?
@burner sorry for poking you like this, but have you made any progress on this?
@RazvanN7 please don't feel sorry, I have not. But I just opened up the std.algorithm.setup. Thank you for being my naggy feeling in the back of my mind !
FWIW: I would prefer if this did not go ahead. We are not really doing this kind of thing with any other invariants and SortedRange is not even able to properly enforce its own invariant due to mutable aliasing. In fact, I would also prefer to have binary search functions in std.algorithm that do not require a sorted range.
