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

Override StepBy::{try_fold, try_rfold} #64121

Merged
merged 1 commit into from
Sep 9, 2019
Merged

Override StepBy::{try_fold, try_rfold} #64121

merged 1 commit into from
Sep 9, 2019

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
Collaborator

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

r? @scottmcm

@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.

Some(x) => acc = f(acc, x)?,
}
}
from_fn(|| self.iter.nth(self.step)).try_fold(acc, f)
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
Contributor

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterating with step_by(1) is much slower than without
6 participants