-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add is_sorted
to Iterator
and [T]
#55045
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_by
should dispatch to[T]::is_sorted_by
instead 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
/libstd
from 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_sorted
itself cannot be inlined if its not#[inline]
as well because the monomorphization happened inlibstd
instead of the user crate since the type ofx
is concrete).
assert!(![-2i32, -1, 0, 3].is_sorted_by_key(|n| n.abs())); | ||
assert!(!["c", "bb", "aaa"].is_sorted()); | ||
assert!(["c", "bb", "aaa"].is_sorted_by_key(|s| s.len())); | ||
} |
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.
assert!(![1, 3, 2].iter().is_sorted()); | ||
assert!([0].iter().is_sorted()); | ||
assert!(std::iter::empty::<i32>().is_sorted()); | ||
assert!(![0.0, 1.0, std::f32::NAN].iter().is_sorted()); |
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