Implement more Iterator methods on core::iter::Repeat by lopopolo · Pull Request #85338 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

lopopolo

core::iter::Repeat always returns the same element, which means we can
do better than implementing most Iterator methods in terms of
Iterator::next.

Fixes #81292.

#81292 raises the question of whether these changes violate the contract of core::iter::Repeat, but as far as I can tell core::iter::repeat doesn't make any guarantees around how it calls Clone::clone.

@lopopolo

core::iter::Repeat always returns the same element, which means we can do better than implementing most Iterator methods in terms of Iterator::next.

Fixes #81292.

@rust-highfive

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

Soveu

}
fn last(self) -> Option {
loop {}

Choose a reason for hiding this comment

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

Wouldn't self.element be the last one?
Just optimized from running infinite time to a no-op :D

Choose a reason for hiding this comment

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

Repeat is an infinite iterator so it has no last element.

Choose a reason for hiding this comment

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

But it has last element according to next_back implementation.

Choose a reason for hiding this comment

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

Can we please have this behavior instead? I am using it to represent something like [[*] in tr where it expands however much is required. To make this easier I need last() to actually return something to act as a fallback in case the sequences in set1 is longer than set2.

Is there something to be gain from this being an infinite loop?

Choose a reason for hiding this comment

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

This code does not change the behavior of repeat when calling last from Repeat as it existed prior to this PR. The default implementation it replaces calls next until None is returned, which never happens for Repeat.

@joshtriplett

@bors

📌 Commit 963bd3b has been approved by joshtriplett

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

May 17, 2021

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

May 18, 2021

@GuillaumeGomez

…gh-81292, r=joshtriplett

Implement more Iterator methods on core::iter::Repeat

core::iter::Repeat always returns the same element, which means we can do better than implementing most Iterator methods in terms of Iterator::next.

Fixes rust-lang#81292.

rust-lang#81292 raises the question of whether these changes violate the contract of core::iter::Repeat, but as far as I can tell core::iter::repeat doesn't make any guarantees around how it calls Clone::clone.

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 18, 2021

@bors

…laumeGomez

Rollup of 7 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

fee1-dead

Comment on lines +82 to +90

fn advance_by(&mut self, n: usize) -> Result<(), usize> {
// Advancing an infinite iterator of a single element is a no-op.
let _ = n;
Ok(())
}
#[inline]
fn nth(&mut self, n: usize) -> Option {
let _ = n;

Choose a reason for hiding this comment

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

@joshtriplett @lopopolo

These two functions, as well as their counterparts in DoubleEndedIterator are incorrect. It changes behavior because there can be an arbitrary Drop impl for A, which means the side effects are removed by this PR.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.