- Notifications
You must be signed in to change notification settings - Fork 13.9k
Add is_sorted to Iterator and [T] #55045
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
Conversation
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.
Apart from my these three nits, LGTM 👍
Thanks for doing this!
But I'm not from the Rust team, so this still needs an official review ^_^
601e657 to b5ab524 Compare | If the use of |
I'd be interested in that as well. So your logic is fine I guess (just inline the two "wrapper" methods). But from my C++ days I still remember that "you shall not tell the compiler when to inline and when not! The compiler is way smarter than you anyway". Furthermore, I think So yes, I'd be really interested in someone explaining what's the best thing to do in this situation ^_^ |
b5ab524 to 566db6f Compare | Triage: Assigning a ~random person from the libs team: r? @KodrAus |
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.
Thanks @kleimkuhler! The updates to the unstable book and the docs on these new methods are really nice 👍
The #[inline] attribute is a bit of a funny one. Like @LukasKalbertodt says it doesn't force inlining, but makes inlining possible across crates if it wasn't already. My understanding is that it's only needed when you want something to be able to be inlined and there are no generics involved. If there are generics then we don't need the attribute, because of the way monomorphization works those methods are already candidates for inlining because they're compiled into the target crate.
566db6f to 6b46008 Compare | Thanks for the explanation on #[inline]! I removed it from the generic methods. That currently leaves all comments addressed/resolved. |
| ☔ The latest upstream changes (presumably #54778) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Major things to resolve:
slice.iter().is_sorted_byshould dispatch to[T]::is_sorted_byinstead ofIterator::is_sorted_by- not enough tests to detect regressions:
- for user-defined types that are only
PartialOrd(like @LukasKalbertodt set example) - floating-point numbers (wanna probably cover all corner cases with
NaN, infinity, and denormals) - types implementing
Ord
- for user-defined types that are only
Minor nitpicks:
- lack of
#[inline]attribute prevents monomorphizations of these methods generated inlibcore/libstdfrom being inlined into user crates (e.g. if#[inline] fn libstd::foo(x: &[u32]) { x.is_sorted() }is inlined into a user's crate,is_sorteditself cannot be inlined if its not#[inline]as well because the monomorphization happened inlibstdinstead of the user crate since the type ofxis concrete).
src/libcore/tests/slice.rs Outdated
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.
We want to add many optimizations in the future, so we should try to already add a comprehensive test suite, e.g., see https://github.com/gnzlbg/is_sorted/tree/master/tests for inspiration, but @LukasKalbertodt RFC and associated discussion also covered many interesting cases.
src/libcore/tests/iter.rs Outdated
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.
@LukasKalbertodt shouldn't is_sorted return true here ?
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.
This was a test case taken from the RFC for a heads up.
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.
@kleimkuhler I think this is correct. These are some other tests from the discussion:
[nan, nan, nan] [1.0, nan, 2.0] [2.0, nan, 1.0] [2.0, nan, 1.0, 7.0] [2.0, nan, 1.0, 0.0] [-nan, -1.0, 0.0, 1.0, nan] [nan, -nan, -1.0, 0.0, 1.0] [1.0, nan, -nan, -1.0, 0.0] [0.0, 1.0, nan, -nan, -1.0] [-1.0, 0.0, 1.0, nan, -nan]IIRC is_sorted does return false for all of these, but is_sorted_by(|a,b| a < b) returns true for some of these. It would be great to cover these for f32 and f64.
@ExpHP also suggested:
// using `Suffix::from` ["a", "aa", "aaa", "b", "bb", "bbb"] [ "", "a", "aa", "", "b", "bb"] // using set / subset: [set![3], set![2]] [set![2], set![3]] [set![2], set![3], set![2, 3]] [set![2], set![2, 3], set![3]] [set![2], set![2, 3], set![5]] [set![2, 3], set![5], set![2]]See this comment (rust-lang/rfcs#2351 (comment)) and the associated gist with the test source code (https://gist.github.com/ExpHP/c23b51f0a9f5f94f2375c93137299604).
6b46008 to 111d245 Compare | The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
111d245 to 045ae2a Compare | Ping from triage @KodrAus / @rust-lang/libs: This PR needs your review. |
| ☔ The latest upstream changes (presumably #56186) made this pull request unmergeable. Please resolve the merge conflicts. |
| @kleimkuhler thanks for your patience! I think we've addressed the comments about |
| @KodrAus Yes sorry for the delay in this; I will address the dispatch issue and add some additional tests shortly. I can close the PR temporarily so that it gets cleared from the triage's monitor and reopen when it is ready again if you would like! |
045ae2a to 2ffdd34 Compare | I'm working on the issue of
pointed out by @gnzlbg . After some input from @LukasKalbertodt I had: and I was getting the following error: Even with adding the lifetime bounds as recommended and getting I went ahead with adding the same lifetime bounds to the I'm not sure I want to keep this PR open much longer since I feel like that is just a bad reflection, but I'm not sure on my next step to address the dispatch issue. Would I be able to get some pointers on what the compiler expects with regards to the lifetime's of the references in the dispatch? |
| The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| That error message rings a bell: in My recommendation is to try to minimize the error in the playground, and try to solve it there first. People on IRC / Discord were very helpful when I was implementing this, so once you have it on the playground, you might want to ask for help there. |
| I reproduced the problem: playground. The error is in fact that the closure passed to self.make_slice().is_sorted_by(|a, b| compare(&a, &b))Because So I see two solutions:
I guess (2.) is the better solution here. But as a general note: I'd love to see this PR get merged soon. In particular, things like "override @gnzlbg Are there any things that in your opinion absolutely need to be done before merging this PR? |
| @kleimkuhler do you plan to rebase this any time soon? Wondering if this should come before or after #56932. |
| @clarcharr Thanks for the ping and sorry about the delay in getting back to you! Just returned home from some travel abroad and catching up on notifications. If you are okay to wait until end of tomorrow, I'll take some time to rebase this and get the checks passing again for a merge. |
| @kleimkuhler sounds good to me! I'll rebase on top of your work once it's done. I figured I'd check on the remaining Iterator PRs and make the offer. :) |
f2b4711 to 29dc8e2 Compare | The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| ☔ The latest upstream changes (presumably #57630) made this pull request unmergeable. Please resolve the merge conflicts. |
Additionally, the root implementation was changed a bit: it now uses `all` instead of coding that logic manually. To avoid duplicate code, the inherent `[T]::is_sorted_by` method now calls `self.iter().is_sorted_by(...)`. This should always be inlined and not result in overhead.
29dc8e2 to c595116 Compare c595116 to b4766f8 Compare | This is at a state that can merge again. There is the point brought up by @gnzlbg about the quality and depth of testing with regards to the optimizations that are down the road for this feature. As pointed out, a more comprehensive test suite was laid out in the RFC comments. Currently, neither Note: A quick pre-merge question I have is about the tests in I think we want to change the I'm not sure right now how to verify that, I'd be interested in figuring out how to confirm that if someone is interested in helping me out with that. Otherwise, sorry for all the back and forth on this PR and hopefully I have not totally stalled implementation of this feature. I'll be interested in the further optimizations that are in place for this once the SIMD work has become stable (if it has not already) and can help with preparations for that. Edit: @KodrAus ping for figuring out whether to merge or not. @clarcharr ping for being dependence from #56932 |
| Thanks for working through this @kleimkuhler! I'll capture the outstanding questions in the tracking issue that can be followed up in future PRs so we can keep the ball rolling. @bors r+ |
| 📌 Commit b4766f8 has been approved by |
Add `is_sorted` to `Iterator` and `[T]` This is an initial implementation for the first step of [RFC 2351](https://github.com/rust-lang/rfcs/blob/master/text/2351-is-sorted.md) Tracking issue: #53485
| ☀️ Test successful - checks-travis, status-appveyor |
…r=cuviper Add more comprehensive tests for is_sorted and friends See rust-lang#53485 and rust-lang#55045.
This is an initial implementation for the first step of RFC 2351
Tracking issue: #53485