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

adapters::cartesian_product does a bit too much #295

Closed
reuvenpo opened this issue Jun 23, 2018 · 2 comments
Closed

adapters::cartesian_product does a bit too much #295

reuvenpo opened this issue Jun 23, 2018 · 2 comments

Comments

@reuvenpo
Copy link

Looking at how pub fn cartesian_product<I, J>(mut i: I, j: J) -> Product<I, J> initialises Product, one can see that it yields the first item in i.
Wouldn't it be better if Product.a_cur was an Option<Option<I::Item>> and initialized as None, rather than initializing it by starting the consumption of i?

This may cause problems if iteration over i has side effects.

Same with Product.b too. No reason to clone it ahead of time.

I haven't looked deep into at the implementation of multi_cartesian_product but it may be a similar situation there too.

@Philippe-Cholet
Copy link
Member

I agree, I came to the same conclusion and same "nested option" solution, implemented in #800. More about laziness in #791. It took a while but it's solved.

@reuvenpo
Copy link
Author

Wow, I have no idea what i could've needed this for back in 2018, but i might actually need it soon for something so... thanks for fixing it - good timing! 😆

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

No branches or pull requests

2 participants