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

Optimize &Iterator::nth #25471

Closed
wants to merge 3 commits into from
Closed

Conversation

Stebalien
Copy link
Contributor

There are actually three changes here:

  1. Remove unnecessary by_ref calls in the Iterator trait. I needed to remove the by_ref call from the nth method to make it object safe (by_ref isn't object safe) but I ended up removing all of them because they were pointless.
  2. ⚠️ Make nth object safe. This changes nth's signature but I'm nearly positive that this change can't break existing code (see the commit message).
  3. Override nth on by_ref iterators to make it call though to nth on the underlying implementation (the type behind the reference). This way, if the underlying implementation provides an optimized nth, by_ref iterators can take advantage of it.

Part of #24214

This changes the signature of nth but can't break any existing code
because:

  1. nth can currently only be overridden on Sized structs.
  2. Extra `Self: Sized` constraints on methods in impls on Sized
     structs are no-ops.
@rust-highfive
Copy link
Collaborator

r? @brson

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

@Stebalien
Copy link
Contributor Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson May 16, 2015
@Stebalien
Copy link
Contributor Author

Feel free to completely ignore this PR till after the 1.0 dust settles.

When treating a reference to an iterator as an iterator, proxy calls to
nth to the underlying implementation instead of calling the default nth
defined in the trait. This allows us to take advantage of optimized nth
implementations in by-ref iterators.

We can't proxy the other methods due to their sized constraints and the
fact that we can't specialize.
@alexcrichton
Copy link
Member

Hm, I agree that I'm not 100% certain that the removal of where Self: Sized is OK to do, but I would personally suspect that it's compatible enough to be ok to land.

@alexcrichton alexcrichton added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 2, 2015
@alexcrichton
Copy link
Member

@Stebalien could you back out the removal of where Self: Sized for Iterator::nth? After some discussion we're thinking that we may want to pursue this at a later date (and perhaps automatically not even require where Self: Sized for object safety), but for now it seems odd to make a one-off method on Iterator object-safe while leaving behind all the others.

@alexcrichton alexcrichton removed the I-needs-decision Issue: In need of a decision. label Jun 10, 2015
@Stebalien
Copy link
Contributor Author

@alexcrichton unfortunately, not without specialization. I would need I: Sized which rust happily informs me I'm not allowed to do:

error: the requirement `I : marker::Sized` appears on the impl method but not on the corresponding trait method [E0276]
fn nth(&mut self, n: usize) -> Option<I::Item> where I: Sized { (**self).nth(n) }

@alexcrichton
Copy link
Member

Yeah the impl of Iterator for &mut I will no longer be able to delegate the nth implementation, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants