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 for Zip #60574

Closed
wants to merge 1 commit into from
Closed

Add custom nth_back for Zip #60574

wants to merge 1 commit into from

Conversation

mdnight
Copy link

@mdnight mdnight commented May 6, 2019

Implementation of nth_back for Zip.
Part of #54054
r? @scottmcm

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2019
@timvermeulen
Copy link
Contributor

Since the underlying iterators may have side-effects, I don't think we can return early when min_len < n.

@koalatux
Copy link
Contributor

koalatux commented May 6, 2019

@timvermeulen, returning early might be ok, especially if you are leveraging the underlaying nth_back. The forward next also does short-circuiting, its documentation sais: "If the first iterator returns None, zip will short-circuit and next will not be called on the second iterator." Maybe we can use a similar behaviour here.

@timvermeulen
Copy link
Contributor

@koalatux That guarantee is specific to Zip, whereas custom implementations of nth and nth_back are required by the Iterator trait to have the same behavior in regards to side effects as the default implementations (i.e. repeated calls to next/next_back).

So short-circuiting Zip::nth_back by forwarding to nth_back on the inner iterators is indeed fine, if done carefully, but short-circuiting based on the simple check min_len < n is not.

@Dylan-DPC-zz
Copy link

ping from triage @scottmcm waiting for your review on this


#[inline]
fn nth_back(&mut self, mut n: usize) -> Option<(A::Item, B::Item)> {
while let Some(x) = ZipImpl::next_back(self) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default implementation of nth_back calls Iterator::next_back in a loop, which seems like it'd be just as good as this one? It looks like ZipImpl::nth exists, so I'd have expected the change here to add ZipImpl::nth_back and specialize it the same way.

@scottmcm scottmcm 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 May 29, 2019
@jonas-schievink
Copy link
Contributor

Ping from triage @mdnight, can you address the review comment?

@JohnCSimon
Copy link
Member

Ping from triage @mdnight please address the review comment

@JohnCSimon
Copy link
Member

@scottmcm Triage: Thanks for the PR but I'm going to close this due to lack of movement.

@JohnCSimon JohnCSimon closed this Jul 27, 2019
@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants