Skip to content
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

Merged
merged 7 commits into from
Jan 21, 2019
Merged

Conversation

kleimkuhler
Copy link
Contributor

This is an initial implementation for the first step of RFC 2351

Tracking issue: #53485

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a 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 ^_^

src/libcore/iter/iterator.rs Outdated Show resolved Hide resolved
src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
@kleimkuhler
Copy link
Contributor Author

If the use of #[inline] can also be confirmed to be correct, I have it for is_sorted and is_sorted_by_key methods, but not the is_sorted_by method (on both Iterator and [T])

@LukasKalbertodt
Copy link
Member

If the use of #[inline] can also be confirmed to be correct, I have it for is_sorted and is_sorted_by_key methods, but not the is_sorted_by method (on both Iterator and [T])

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 #[inline] in Rust is also different than one might expect -- it's by no means forcing the compiler to inline it. It just makes the function "available" when compiling other crates (as in: it will be recompiled for every crate that uses it). But I think it's not necessary here as all generic functions are available in the same sense anyway. But I might be very wrong, given that many slice methods are marked #[inline].

So yes, I'd be really interested in someone explaining what's the best thing to do in this situation ^_^

@TimNN TimNN added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2018
@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2018

Triage: Assigning a ~random person from the libs team: r? @KodrAus

Copy link
Contributor

@KodrAus KodrAus left a 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.

src/libcore/iter/iterator.rs Show resolved Hide resolved
src/libcore/slice/mod.rs Show resolved Hide resolved
@kleimkuhler
Copy link
Contributor Author

Thanks for the explanation on #[inline]! I removed it from the generic methods. That currently leaves all comments addressed/resolved.

@bors
Copy link
Contributor

bors commented Oct 23, 2018

☔ The latest upstream changes (presumably #54778) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@gnzlbg gnzlbg left a 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 of Iterator::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

Minor nitpicks:

  • lack of #[inline] attribute prevents monomorphizations of these methods generated in libcore/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 in libstd instead of the user crate since the type of x is concrete).

src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
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()));
}
Copy link
Contributor

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 Show resolved Hide resolved
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());
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

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).

src/libcore/iter/iterator.rs Show resolved Hide resolved
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:08154cd2:start=1542087386184804549,finish=1542087387294290778,duration=1109486229
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0

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 @TimNN. (Feature Requests)

src/libcore/tests/lib.rs Outdated Show resolved Hide resolved
@TimNN
Copy link
Contributor

TimNN commented Nov 20, 2018

Ping from triage @KodrAus / @rust-lang/libs: This PR needs your review.

@bors
Copy link
Contributor

bors commented Nov 23, 2018

☔ The latest upstream changes (presumably #56186) made this pull request unmergeable. Please resolve the merge conflicts.

@KodrAus
Copy link
Contributor

KodrAus commented Nov 26, 2018

@kleimkuhler thanks for your patience! I think we've addressed the comments about #[inline] now. So we just need to extend the test suite a bit with some of the curlier cases that @gnzlbg described and I think we should be good to go!

@kleimkuhler
Copy link
Contributor Author

@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!

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2018
@kleimkuhler
Copy link
Contributor Author

I'm working on the issue of

slice.iter().is_sorted_by should dispatch to [T]::is_sorted_by instead of Iterator::is_sorted_by

pointed out by @gnzlbg . After some input from @LukasKalbertodt I had:

#[inline]
fn is_sorted_by<F>(mut self, compare: F) -> bool
where
    Self: Sized,
    F: FnMut(&Self::Item, &Self::Item) -> Option<Ordering>
{
    self.make_slice().is_sorted_by(compare)
}

and I was getting the following error:

error[E0277]: expected a `ops::function::FnMut<(&T, &T)>` closure, found `F`23: core
    --> src/libcore/slice/mod.rs:3048:35
     |
3048 |                 self.make_slice().is_sorted_by(compare)
     |                                   ^^^^^^^^^^^^ expected an `FnMut<(&T, &T)>` closure, found `F`
...
3187 | iterator!{struct Iter -> *const T, &'a T, const, /* no mut */}
     | -------------------------------------------------------------- in this macro invocation
     |
     = help: the trait `for<'r, 's> ops::function::FnMut<(&'r T, &'s T)>` is not implemented for `F`
     = help: consider adding a `where for<'r, 's> F: ops::function::FnMut<(&'r T, &'s T)>` bound

Even with adding the lifetime bounds as recommended and getting for<'r, 's> F: FnMut(&'r Self::Item, &'s Self::Item) -> Option<Ordering>, I keep getting the error that the trait is not satisfied for F.

I went ahead with adding the same lifetime bounds to the fn is_sorted_by, as well as played around with the use of references and lifetimes in different places. I tried reproducing a more minimal example, but with the fact that these errors occur during the macro invocation on #[inline] methods I have not been too successful with that.

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?

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2386383b:start=1543981104451121704,finish=1543981106611940530,duration=2160818826
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:46:29] .................................................................................................... 1100/5107
[00:46:32] .................................................................................................... 1200/5107
[00:46:34] .................................................................................................... 1300/5107
[00:46:36] .................................................................................................... 1400/5107
[00:46:38] ........................................F........................................................... 1500/5107
[00:46:45] .................................................................................................... 1700/5107
[00:46:48] .................................................................................................... 1800/5107
[00:46:51] .................................................................................................... 1900/5107
[00:46:54] ..................................i................................................................. 2000/5107

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 @TimNN. (Feature Requests)

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 5, 2018

That error message rings a bell: in is_sorted I solved it this way back then, but one might need to do something different here.

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.

@LukasKalbertodt
Copy link
Member

I reproduced the problem: playground. The error is in fact that the closure passed to <slice::Iter as Iterator>::is_sorted_by receives arguments of type &&T (&Self::Item where Self::Item is &T). Sadly, we can't just write:

self.make_slice().is_sorted_by(|a, b| compare(&a, &b))

Because [T]::is_sorted_by takes F: FnMut(&T, &T) -> Option<Ordering>, meaning that the references the closure is called with can have any lifetime, no particularly 'a. But since Iterator requires F: FnMut(&Self::Item, &Self::Item) which in our case means FnMut(&&'a T, &&'a T) (meaning: requiring exactly lifetime 'a), we get a lifetime mismatch. In other words: while the closure of [T]::is_sorted_by has to be able to deal with any lifetime, the closure passed to Iterator::is_sorted_by can only deal with the lifetime 'a, so we can't implement the first with the second. This is also why I think @gnzlbg solution won't work in this situation, but I am not entirely sure!

So I see two solutions:

  1. Change the interface of [T]::is_sorted_by to:

    fn is_sorted_by<'a, F>(&'a self, mut compare: F) -> bool
    where
        F: FnMut(&'a T, &'a T) -> Option<Ordering>

    This "relaxes" the API as in: the caller has to provide less. But now the calle (is_sorted_by) really has to only pass 'a references into compare. Which is ok now, but I'm not sure it will always be OK.

  2. Create a private function/method with the signature shown above while keeping the public interface as specified in the RFC. Then both, [T]::is_sorted_by and <slice::iter as Iterator>::is_sorted_by would use that internal function.

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 Iterator::is_sorted_by for slice::Iter" can very well be done in subsequent PRs and shouldn't really be blocking this one. Sticking with a PR for a long time can be exhausting, in particular as a new contributor (I know what I'm talking about :P). So if the overwrite-iterator thingy can't be solved easily now, I would just drop that change for this PR.

@gnzlbg Are there any things that in your opinion absolutely need to be done before merging this PR?

@clarfonthey
Copy link
Contributor

@kleimkuhler do you plan to rebase this any time soon? Wondering if this should come before or after #56932.

@kleimkuhler
Copy link
Contributor Author

@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.

@clarfonthey
Copy link
Contributor

@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. :)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0adfb1d6:start=1547533872111688181,finish=1547533873240624492,duration=1128936311
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:20:08] .................................................................................................... 1200/5303
[01:20:10] .................................................................................................... 1300/5303
[01:20:13] .................................................................................................... 1400/5303
[01:20:16] .................................................................................................... 1500/5303
[01:20:20] .......F..................................................................................i......... 1600/5303
[01:20:27] .................................................................................................... 1800/5303
[01:20:32] .................................................................................................... 1900/5303
[01:20:36] .................................................................................................... 2000/5303
[01:20:39] ................i................................................................................... 2100/5303
---
[01:22:58] failures:
[01:22:58] 
[01:22:58] ---- [ui] ui/feature-gates/feature-gate-is_sorted.rs stdout ----
[01:22:58] 
[01:22:58] error: /checkout/src/test/ui/feature-gates/feature-gate-is_sorted.rs:3: unexpected error: '3:33: 3:42: use of unstable library feature 'is_sorted': new API (see issue #53485) [E0658]'
[01:22:58] 
[01:22:58] error: /checkout/src/test/ui/feature-gates/feature-gate-is_sorted.rs:5: unexpected error: '5:39: 5:55: use of unstable library feature 'is_sorted': new API (see issue #53485) [E0658]'
[01:22:58] 
[01:22:58] error: /checkout/src/test/ui/feature-gates/feature-gate-is_sorted.rs:9: unexpected error: '9:26: 9:35: use of unstable library feature 'is_sorted': new API (see issue #53485) [E0658]'
[01:22:58] 
[01:22:58] error: /checkout/src/test/ui/feature-gates/feature-gate-is_sorted.rs:11: unexpected error: '11:32: 11:48: use of unstable library feature 'is_sorted': new API (see issue #53485) [E0658]'
[01:22:58] error: 4 unexpected errors found, 0 expected errors not found
[01:22:58] status: exit code: 1
[01:22:58] status: exit code: 1
[01:22:58] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/feature-gates/feature-gate-is_sorted.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-is_sorted/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-is_sorted/auxiliary" "-A" "unused"
[01:22:58]     Error {
[01:22:58]         line_num: 3,
[01:22:58]         kind: Some(
[01:22:58]             Error
[01:22:58]             Error
[01:22:58]         ),
[01:22:58]         msg: "3:33: 3:42: use of unstable library feature \'is_sorted\': new API (see issue #53485) [E0658]"
[01:22:58]     Error {
[01:22:58]         line_num: 5,
[01:22:58]         kind: Some(
[01:22:58]             Error
[01:22:58]             Error
[01:22:58]         ),
[01:22:58]         msg: "5:39: 5:55: use of unstable library feature \'is_sorted\': new API (see issue #53485) [E0658]"
[01:22:58]     Error {
[01:22:58]         line_num: 9,
[01:22:58]         kind: Some(
[01:22:58]             Error
[01:22:58]             Error
[01:22:58]         ),
[01:22:58]         msg: "9:26: 9:35: use of unstable library feature \'is_sorted\': new API (see issue #53485) [E0658]"
[01:22:58]     Error {
[01:22:58]         line_num: 11,
[01:22:58]         kind: Some(
[01:22:58]             Error
[01:22:58]             Error
[01:22:58]         ),
[01:22:58]         msg: "11:32: 11:48: use of unstable library feature \'is_sorted\': new API (see issue #53485) [E0658]"
[01:22:58] ]
[01:22:58] 
[01:22:58] thread '[ui] ui/feature-gates/feature-gate-is_sorted.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:1342:13
[01:22:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[01:22:58] 
[01:22:58] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:495:22
[01:22:58] 
[01:22:58] 
[01:22:58] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:22:58] 
[01:22:58] 
[01:22:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:22:58] Build completed unsuccessfully in 0:04:51
[01:22:58] Build completed unsuccessfully in 0:04:51
[01:22:58] Makefile:48: recipe for target 'check' failed
[01:22:58] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2348e4c8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Jan 15 07:54:22 UTC 2019
---
travis_time:end:20a3fd02:start=1547538863854479259,finish=1547538863862378013,duration=7898754
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02d29666
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0e4fa251
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Jan 15, 2019

☔ The latest upstream changes (presumably #57630) made this pull request unmergeable. Please resolve the merge conflicts.

kleimkuhler and others added 6 commits January 17, 2019 22:34
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.
@kleimkuhler
Copy link
Contributor Author

kleimkuhler commented Jan 19, 2019

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 tests/slice.rs or tests/iter.rs have macro generating methods. Would it make sense to add those to tests/lib.rs since they may use both? Is it something that should also be included in this PR?

Note: A quick pre-merge question I have is about the tests in tests/iter.rs. Right now they look like this assert!([some slice].iter().is_sorted());. After all the discussion that took place above, my understanding is that .iter() is returning an Iter<'a, T> which dispatches to the [T]'s is_sorted().

I think we want to change the tests/iter.rs to assert!([some slice].as_mut_ptr().into_iter().is_sorted()); so that it returns an IterMut<'a, T> and dispatches to the Iterator's is_sorted().

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

@KodrAus
Copy link
Contributor

KodrAus commented Jan 21, 2019

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+

@bors
Copy link
Contributor

bors commented Jan 21, 2019

📌 Commit b4766f8 has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2019
@bors
Copy link
Contributor

bors commented Jan 21, 2019

⌛ Testing commit b4766f8 with merge 7164a9f...

bors added a commit that referenced this pull request Jan 21, 2019
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
@bors
Copy link
Contributor

bors commented Jan 21, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: KodrAus
Pushing 7164a9f to master...

@bors bors merged commit b4766f8 into rust-lang:master Jan 21, 2019
bluebear94 added a commit to bluebear94/rust that referenced this pull request Jun 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 21, 2023
…r=cuviper

Add more comprehensive tests for is_sorted and friends

See rust-lang#53485 and rust-lang#55045.
chenyukang pushed a commit to chenyukang/rust that referenced this pull request Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.