Skip to content

Conversation

@timvermeulen
Copy link
Contributor

Previous PR: #51435

The previous PR was closed in favor of #51601, which was later reverted. I don't think these implementations will make it harder to specialize StepBy<Range<_>> later, so we should be able to land this without any consequences.

This should fix #57517 – in my benchmarks iter and iter.step_by(1) now perform equally well, provided internal iteration is used.

@rust-highfive
Copy link
Contributor

r? @KodrAus

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

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

Centril commented Sep 3, 2019

@rust-highfive rust-highfive assigned scottmcm and unassigned KodrAus Sep 3, 2019
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This definitely seems like one that needs try_fold (though I wish LLVM would just peel it). One minor change and it'll be good to go.

Copy link
Member

Choose a reason for hiding this comment

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

Can you just use a while let Some(x) = self.iter.nth(self.step) loop here instead? It'll do the same thing, but without introducing a new closure that would regress #62429.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I forgot about that. I turned the closures into functions instead, that should have the same benefits, correct? I figured this will make it easier to potentially undo these changes in the future.

@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 Sep 4, 2019
@scottmcm
Copy link
Member

scottmcm commented Sep 9, 2019

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 9, 2019

📌 Commit 78908f2 has been approved by scottmcm

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 9, 2019
… r=scottmcm Override `StepBy::{try_fold, try_rfold}` Previous PR: rust-lang#51435 The previous PR was closed in favor of rust-lang#51601, which was later reverted. I don't think these implementations will make it harder to specialize `StepBy<Range<_>>` later, so we should be able to land this without any consequences. This should fix rust-lang#57517 – in my benchmarks `iter` and `iter.step_by(1)` now perform equally well, provided internal iteration is used.
bors added a commit that referenced this pull request Sep 9, 2019
Rollup of 5 pull requests Successful merges: - #63468 (Resolve attributes in several places) - #64121 (Override `StepBy::{try_fold, try_rfold}`) - #64278 (check git in bootstrap.py) - #64306 (Fix typo in config.toml.example) - #64312 (Unify escape usage) Failed merges: r? @ghost
@bors bors merged commit 78908f2 into rust-lang:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

6 participants