Skip to content

specialize Iterator::nth for Cycles over ExactSizeIterators to #47555

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

Closed
wants to merge 5 commits into from

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Jan 18, 2018

reduce clones at the cost of one remainder. This should be faster in most cases and may in some situations reduce memory churn via clones.

reduce clones at the cost of one remainder. This should be faster
in most cases and may in some situations reduce memory churn via
clones.
@rust-highfive
Copy link
Contributor

r? @withoutboats

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

@varkor
Copy link
Member

varkor commented Jan 18, 2018

This is subject to the same discussion as in #47533, concerning observability of clones.

@withoutboats
Copy link
Contributor

@bors r? @bluss

@rust-highfive rust-highfive assigned bluss and unassigned withoutboats Jan 18, 2018
#[stable(feature = "rust1", since = "1.0.0")]
impl<I> Iterator for Cycle<I>
where I: Clone + Iterator + ExactSizeIterator,
<I as Iterator>::Item : Clone + Copy {
Copy link
Member

Choose a reason for hiding this comment

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

This should be just I::Item: Copy style wise. Iterator + ExactSizeIterator is also redundant, just ExactSizeIterator is the same.

@bluss
Copy link
Member

bluss commented Jan 20, 2018

Omitting Clone calls for Copy types is fine, that's something we've decided previously (I just don't have a link for that).

Current implementation omits the clones of the iterator itself, so a motivation is needed for that too.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2018
@llogiq
Copy link
Contributor Author

llogiq commented Jan 21, 2018

Thank you for the review! We require clone impls for Copy types to be a memcpy, so eliding them should be OK.

Regarding the iterator, I can change it to require Copy, too, to be on the safe side. On the other hand it is debatable whether cloning the iterator should be an observable side effect of the implementation.

@durka
Copy link
Contributor

durka commented Jan 21, 2018

Iterators aren't Copy as policy so this impl isn't likely to apply to much of anything.

@llogiq
Copy link
Contributor Author

llogiq commented Jan 21, 2018

Too true, I better revert that one. So since we cannot require I: Copy, we can either declare the number of clones (and thus drops) on the iterator implementation-specified and try to reduce them, or leave stuff as is and have people deal with it in other ways (i.e. by restricting the n parameter manually).

@shepmaster
Copy link
Member

Ping from triage, @bluss — new commits up for you to review!

@bluss
Copy link
Member

bluss commented Jan 26, 2018

I think the same applies as in the other PRs, see this comment #47533 (comment)

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2018
@kennytm
Copy link
Member

kennytm commented Jan 31, 2018

Ping from triage @bluss @llogiq!

Given the linked comment #47533 (comment), I guess this PR should be closed since clones of the iterator itself are still elided, and an RFC detailing whether clone elision is acceptable should be accepted before this PR can be reconsidered?

@llogiq
Copy link
Contributor Author

llogiq commented Jan 31, 2018

Until we all reach decision
On iterator clone elision
I'm going to close
this PR so those
that remain are awaiting revision

@llogiq llogiq closed this Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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