search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

Simplify `Permutations` #790

Closed Philippe-Cholet closed 11 months ago

Philippe-Cholet commented 11 months ago

Fixes #747

This is quite a rewrite, very little is untouched. count and size_hint are really simplified with the new PermutationState::size_hint_for.

cargo bench permutation
// Before this PR
permutations iter       [62.130 µs 62.355 µs 62.620 µs]
permutations range      [61.637 µs 61.850 µs 62.104 µs]
permutations slice      [64.177 µs 64.314 µs 64.459 µs]
// Now
permutations iter       [57.280 µs 57.478 µs 57.702 µs]    -8.4%
permutations range      [59.714 µs 59.914 µs 60.138 µs]    -4.7%
permutations slice      [59.584 µs 59.720 µs 59.868 µs]    -7.1%

I did not expect any performance improvement, but nice to have a little one.

Philippe-Cholet commented 11 months ago

@phimuemue I'm sorry you had some hard time reviewing it. I definitely tried and struggled to make more fine-grained commits here. But I should have thought of your very helpful intermediary Loaded(CompleteState) -> LoadedStart, LoadedOngoing. I'm a work in progress (especially git), and to improve myself I splitted it again myself, trying to leave the last commit intact (FusedIterator, but I git-failed so I had to write it again).

You can check that there is no difference between our branches: https://github.com/Philippe-Cholet/itertools/compare/permutations-states-2..phimuemue:rust-itertools:permutations-states-2 (I've just learnt that .. and ... have different meaning in this url).

phimuemue commented 11 months ago

@phimuemue I'm sorry you had some hard time reviewing it. I definitely tried and struggled to make more fine-grained commits here. But I should have thought of your very helpful intermediary Loaded(CompleteState) -> LoadedStart, LoadedOngoing. I'm a work in progress (especially git), and to improve myself I splitted it again myself, trying to leave the last commit intact (FusedIterator, but I git-failed so I had to write it again).

You can check that there is no difference between our branches: https://github.com/Philippe-Cholet/itertools/compare/permutations-states-2..phimuemue:rust-itertools:permutations-states-2 (I've just learnt that .. and ... have different meaning in this url).

No worries. I hope it didn't come across an offense. Thanks again for taking the time to simplify this.

Philippe-Cholet commented 11 months ago

@phimuemue I did not see any offense, only the opportunity to improve the way I write commits.