itertools
itertools copied to clipboard
Rewrite the multi cartesian product iterator
This PR rewrites the multi cartesian product iterator. The original goal was to fix #337 ; however, there was no nice way of storing the state associated with that (besides just adding a bool that got checked everywhere). The new implementation fixes that bug, is slightly faster on benchmarks (5% and 0%), and hopefully easier to maintain since it is simpler.
Hi there, this could be a great PR! Apparently, it currently uses core::mem::take
, which is not available in our minimum supported rust version. Possibly use replace
instead.
If you touch the code anyway, I think we'd greatly appreciate if you could split up the PR into smaller commits (just to simplify the review (as you probably saw, there's currently not much going on in this crate, so every simplification to the reviews is more than welcome)). Of course, you do not have to do this, but it would probably make review a bit easier. If you do so, here are some suggestions to factor out into separate commits:
- All formatting/indentation changes into one commit.
- Before doing the actual work, inline
fn reset
in one separate commit - it is only used in one place and so thin that it does not need an own function.
I've fixed the usage of core::mem::take
. Hopefully this version should be slightly easier to review; extracting out the formatting changes cleaned up the diff on the big commit somewhat. Fundamentally though, the key data structures and the bodies of all the key functions have been replaced, so I don't think I can productively split the commit up any more. Even inlining reset
doesn't really make sense as a separate commit; in the current diff the function is just removed and its inside ends up in the big blob that is the new implementation of .next()
. If you have any suggestions for what else I could do to make this easier to review, I'd be happy to help!
All I did for now is: drop the "fmt" commit, rebase resolving "fmt" changes and a ;
error, improve the added test a bit.
@jswrenn @phimuemue This rewrite intend to fix a long-time bug but I think I could fuse the iterator as well with this PR by changing the Restarted
variant to an End
variant (EDIT: or a bit different rewrite), would that be okay?
The specialization test is currently ignored as it fails because
The iterator is not fused even when all child iterators are, and this was the intended behavior, that's confusing.