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

make Skip a DoubleEndedIterator #31700

Merged
merged 1 commit into from
Mar 5, 2016
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 16, 2016

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@bluss
Copy link
Member

bluss commented Feb 16, 2016

Implementation lgtm, but is it too surprising that it does all the skip steps when the back element is taken?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2016

not doing that would require ExactSizeIterator

@alexcrichton
Copy link
Member

This also looks good to me, but this is touching a pretty core iterator so I'm going to tag as T-libs just to be sure we discuss it a bit.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 16, 2016
@oli-obk oli-obk changed the title make skip a double ended iterator make Skip a DoubleEndedIterator Feb 18, 2016
@alexcrichton
Copy link
Member

We discussed this in the libs team meeting and the conclusions we reached were:

  • The order of iteration here is probably a little too surprising. Mainly a Skip iterator drained via next_back would be consumed differently than if the underlying iterator were drained via next_back.
  • We may be able to implement this, however, via an ExactSizeIterator bound.
  • There's precedent for double-ended iteration requiring an exact size bound via the rposition method.

@oli-obk would you be ok updating this to also include an ExactSizeIterator bound? That way the next_back method could avoid skipping on the front unless necessary.

@alexcrichton alexcrichton removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 3, 2016
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 4, 2016

It's always backwards compatible to remove the bound in the future, so adding the bound now is fine with me.

Sadly this makes it useless for iterating over str slices by chars

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 4, 2016

updated and added more tests

@alexcrichton
Copy link
Member

@bors: r+ 25e5de3

Thanks @oli-obk!

@bors
Copy link
Contributor

bors commented Mar 5, 2016

⌛ Testing commit 25e5de3 with merge 07a1803...

@bors bors merged commit 25e5de3 into rust-lang:master Mar 5, 2016
@oli-obk oli-obk deleted the skip_double_ended branch March 7, 2016 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants