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 custom nth_back to Skip #60454

Merged
merged 1 commit into from Jun 20, 2019

Conversation

Projects
None yet
9 participants
@acrrd
Copy link
Contributor

commented May 1, 2019

Implementation of nth_back for Skip.
Part of #54054

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

r? @cramertj

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

@cramertj

This comment has been minimized.

Copy link
Member

commented May 1, 2019

r? @scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned cramertj May 1, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Looks correct to me, though I don't think we want nth_back to skip any of the elements at the front, similar to how next_back doesn't skip any. I think we can call self.iter.nth_back(n) as long as n < self.len(), without having to involve self.n.

@acrrd acrrd force-pushed the acrrd:issues/54054_skip branch 5 times, most recently from a516851 to 1edecac May 2, 2019

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Didn't notice that, I changed it

@scottmcm

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Look good, thanks!

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

📌 Commit 1edecac has been approved by scottmcm

@bors

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

⌛️ Testing commit 1edecac with merge 8e9bb42...

bors added a commit that referenced this pull request May 9, 2019

Auto merge of #60454 - acrrd:issues/54054_skip, r=scottmcm
Add custom nth_back to Skip

Implementation of nth_back for Skip.
Part of #54054
if n < self.len() {
self.iter.nth_back(n)
} else {
None

This comment was marked as resolved.

Copy link
@sinkuu

sinkuu May 9, 2019

Contributor

I guess codes like this will be broken:

let mut it = vec![1, 2, 3, 4].into_iter().skip(1);
assert!(it.nth_back(3) == None); // => NOP with this PR
assert!(it.nth_back(0) == None); // => `Some(4)`?
@scottmcm

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@bors r-

Good point, @sinkuu; Thanks!

@acrrd Can you add some more tests for bigger n, and maybe some with by_ref to ensure behavior doesn't change here?

@kennytm

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@bors r- retry

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@sinkuu thanks!

@scottmcm I added the test for a bigger n and another test that check that nth_back behave as a non skipped iterator. It's not clear to me how to do the tests you meant using by_ref

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

I don't think we want nth_back to skip any of the elements at the front, similar to how next_back doesn't skip any

Isn't this still a concern with the current implementation?

@acrrd acrrd force-pushed the acrrd:issues/54054_skip branch 2 times, most recently from b7fe202 to edd4879 May 27, 2019

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@timvermeulen thanks :)

fn nth_back(&mut self, n: usize) -> Option<I::Item> {
if n < self.len() {
self.iter.nth_back(n)
} else {

This comment has been minimized.

Copy link
@sinkuu

sinkuu May 28, 2019

Contributor

In case n >= self.len() && self.len() != 0, additionally you need to do self.iter.nth_back(self.len() - 1); (or whatever that takes self.len()-items from the back) since the underlying iterator could have side effects.

let mut v = vec![];
let mut it = [1, 2, 3, 4, 5]
    .iter()
    .cloned()
    .inspect(|x| v.push(*x))
    .skip(1);
assert_eq!(it.len(), 4);
assert_eq!(it.nth_back(4), None);
assert_eq!(v, vec![5, 4, 3, 2]);

This comment has been minimized.

Copy link
@acrrd

acrrd May 28, 2019

Author Contributor

Why let _ = and not just the call to nth_back?

This comment has been minimized.

Copy link
@sinkuu

sinkuu May 28, 2019

Contributor

It was just for expressing intent to throw away the return value of nth_back, and I don't meant it is actually needed.

@@ -2280,6 +2280,32 @@ fn test_skip_try_folds() {
assert_eq!(iter.next_back(), Some(24));
}

#[test]
fn test_skip_nth_back() {

This comment has been minimized.

Copy link
@scottmcm

scottmcm May 29, 2019

Member

I don't think the comment above was addressed; if disagree please add a test that demonstrates that the underlying iterator is still advanced.

(You could do that with inspect, or something like let mut it = ...; it.by_ref().skip(10).nth_back(100); assert_eq!(it.next_back(), ...);)

This comment has been minimized.

Copy link
@acrrd

acrrd May 29, 2019

Author Contributor

Ok, I didn't understand you meant that.

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen May 29, 2019

Contributor

You're now calling self.len() three times, maybe it's better to store it in a variable first.

@acrrd acrrd force-pushed the acrrd:issues/54054_skip branch 2 times, most recently from 8e83410 to 2966f58 May 29, 2019

@acrrd acrrd force-pushed the acrrd:issues/54054_skip branch from 2966f58 to e80a375 May 29, 2019

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Ping from triage, @acrrd what's the status of this? Are there still open review comments to be addressed, or is this waiting on someone to approve?

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@jonas-schievink I think all comments are addressed, waiting for approval.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Sorry for the delay re-reviewing, this looks great now!

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

📌 Commit e80a375 has been approved by scottmcm

Centril added a commit to Centril/rust that referenced this pull request Jun 20, 2019

Rollup merge of rust-lang#60454 - acrrd:issues/54054_skip, r=scottmcm
Add custom nth_back to Skip

Implementation of nth_back for Skip.
Part of rust-lang#54054

bors added a commit that referenced this pull request Jun 20, 2019

Auto merge of #61983 - Centril:rollup-wnfo07y, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #60454 (Add custom nth_back to Skip)
 - #60772 (Implement nth_back for slice::{Iter, IterMut})
 - #61782 (suggest tuple struct syntax)
 - #61968 (rustc: disallow cloning HIR nodes.)

Failed merges:

r? @ghost

@bors bors merged commit e80a375 into rust-lang:master Jun 20, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
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.