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

Positions based on Enumerate #816

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Dec 8, 2023

Following #813 (comment), with this PR, Positions now uses Enumerate internally.

  • next and next_back are now based on find_map instead of "while-next" loops. It would be useful performance-wise for iterators specializing try_[r]fold. I intend to do the same for other while-next loops (where benchmarks show times decrease, up to -20%).
  • Stable benchmarks here.
  • Doing this, I first made a silly mistake. I therefore add a test that would have helped me, and fix a little error in the documentation (it could alternatively use filter_map to avoid * copy).

`Positions` is tested to be fused, for specialized methods and has two doctests. No other test.
While I was adding `Enumerate` in `Positions` internals, running `cargo test positions` told me `rfold` was wrong. The silly mistake was on `next_back` instead and this test would have helped me.
Plus, I noticed that the documentation was just a bit wrong.
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thank you, @Philippe-Cholet

@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Dec 10, 2023
Merged via the queue into rust-itertools:master with commit cfb2774 Dec 10, 2023
8 checks passed
@Philippe-Cholet Philippe-Cholet deleted the positions-with-enumerate branch December 10, 2023 18:07
@Philippe-Cholet Philippe-Cholet added this to the next milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants