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

Implement DoubleEndedIterator for iter::{StepBy, Peekable, Take} #61457

Open
wants to merge 1 commit into
base: master
from

Conversation

@timvermeulen
Copy link
Contributor

commented Jun 2, 2019

Now that DoubleEndedIterator::nth_back has landed, StepBy and Take can have an efficient DoubleEndedIterator implementation. I don't know if there was any particular reason for Peekable not having a DoubleEndedIterator implementation, but it's quite trivial and I don't see any drawbacks to having it.

I'm not very happy about the implementation of Peekable::try_rfold, but I didn't see another way to only take the value out of self.peeked in case self.iter.try_rfold didn't exit early.

I only added Peekable::rfold (in addition to try_rfold) because its Iterator implementation has both fold and try_fold (and for similar reasons I added Take::try_rfold but not Take::rfold). Do we have any guidelines on whether we want both? If we do want both, maybe we should investigate which iterator adaptors override try_fold but not fold and add the missing implementations. At the moment I think that it's better to always have iterator adaptors implement both, because some iterators have a simpler fold implementation than their try_fold implementation.

The tests that I added may not be sufficient because they're all just existing tests where next/nth/fold/try_fold are replaced by their DEI counterparts, but I do think all paths are covered. Is there anything in particular that I should probably also test?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2019

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 2, 2019

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:05e0c770:start=1559476913973595770,finish=1559476915977649711,duration=2004053941
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:42] tidy error: /checkout/src/libcore/tests/iter.rs:903: trailing whitespace
[00:04:47] some tidy checks failed
[00:04:47] 
[00:04:47] 
[00:04:47] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:47] 
[00:04:47] 
[00:04:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:47] Build completed unsuccessfully in 0:01:14
---
travis_time:end:0015c48c:start=1559477216064562327,finish=1559477216069687930,duration=5125603
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:36606ce6
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:007a8730
travis_time:start:007a8730
$ 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:02421694
$ 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)

}
}

#[unstable(feature = "double_ended_step_by_iterator", issue = "0")]

This comment has been minimized.

Copy link
@kennytm

kennytm Jun 2, 2019

Member

An impl is unfortunately insta-stable.

@kennytm

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

@rust-highfive rust-highfive assigned scottmcm and unassigned kennytm Jun 2, 2019

@timvermeulen timvermeulen force-pushed the timvermeulen:double_ended_iters branch from 4103254 to dd17a6b Jun 2, 2019

impl<I> DoubleEndedIterator for Peekable<I> where I: DoubleEndedIterator {
#[inline]
fn next_back(&mut self) -> Option<Self::Item> {
self.iter.next_back().or_else(|| self.peeked.take().and_then(|x| x))

This comment has been minimized.

Copy link
@scottmcm

scottmcm Jun 2, 2019

Member

If Peekable is going to be DEI, then should it have a peek_back() too? But that has space cost, which might be why it wasn't already double-ended?

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen Jun 2, 2019

Author Contributor

Yeah, that may be why this implementation was missing, but I don't think it needs to have a peek_back() method for it to be useful as a DEI. If anyone needs to be able to peek from both sides, a separate type would probably make more sense because of the memory implications you mentioned.

The only surprising thing about it being a DEI would perhaps be that nth_back can't be implemented efficiently.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Is there anything in particular that I should probably also test?

Your tryfolds look pretty good, but because I've seen so many with short-circuiting issues, could you add some very blatant short-circuit-every-step tests? For example,

    let mut it = (2..20).take(3);
    assert_eq!(it.try_for_each(Err), Err(2));
    assert_eq!(it.try_for_each(Err), Err(3));
    assert_eq!(it.try_for_each(Err), Err(4));
    assert_eq!(it.try_for_each(Err), Ok(()));

    let mut it = (2..20).take(3).rev();
    assert_eq!(it.try_for_each(Err), Err(4));
    assert_eq!(it.try_for_each(Err), Err(3));
    assert_eq!(it.try_for_each(Err), Err(2));
    assert_eq!(it.try_for_each(Err), Ok(()));

(Basically the same as the .next()-repeatedly tests, just spelling .next() as .try_for_each(Err).err() and .next_back() as .try_rfold((), |_, x| Err(x)).err() 🙂)

@timvermeulen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

@scottmcm Good idea, something like this?


#[inline]
fn nth_back(&mut self, n: usize) -> Option<Self::Item> {
self.iter.nth_back(self.next_back_index() + n * (self.step + 1))

This comment has been minimized.

Copy link
@ollie27

ollie27 Jun 5, 2019

Contributor

Couldn't this multiplication and addition overflow?

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen Jun 5, 2019

Author Contributor

Ah, I naively assumed that n would be in-bounds here. Thanks!

@timvermeulen timvermeulen force-pushed the timvermeulen:double_ended_iters branch from db28457 to c79eaaa Jun 5, 2019

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Nominating to see if libs in interested in these.

@timvermeulen timvermeulen force-pushed the timvermeulen:double_ended_iters branch from c79eaaa to 56ebfb1 Jul 9, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Sorry for the delay on the libs side of things, but the libs team discussed this today and is ok adding these impls, so feel free to r+ when ready @scottmcm!

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@alexcrichton Doesn't this need an FCP because it's adding insta-stable impls?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Sure, can go through that as well!

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Jul 11, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.