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

Specialize ProcessResults::fold #563

Merged
merged 2 commits into from Aug 20, 2021

Conversation

SkiFire13
Copy link
Contributor

Partially fixes #562. try_fold can't be added right now because it requires the unstable Try trait.

@phimuemue
Copy link
Member

Thanks a lot for this - looks very good to me (as always with your contributions).

And even if I think this is correct, I think we do ourselves a favor if we have tests for this specialization. Could you add some? (I think we already have quicktests for specializations.)

@SkiFire13
Copy link
Contributor Author

SkiFire13 commented Aug 20, 2021

If you mean tests/specializations.rs I wonder if those tests are even doing something. The mapper function gets a Box<dyn Iterator<Item = IterItem> + 'a> which is (almost) unspecialized anyway due to the implementation of Iterator for Box<dyn Iterator>. Edit: just actually checked by making MapSpecialCase::fold immediately return the accumulator, the only test that failed was the one that didn't use check_specialized

Looks like there's also a tests/fold_specialization.rs, but that doesn't have quicktests.

bors bot added a commit that referenced this pull request Aug 20, 2021
574: Fix and refactor specializations tests r=phimuemue a=SkiFire13

As found in #563 specializations tests don't actually test the specialize tests due to the use of `Box<dyn Iterator>`. This PR fix this problem by having `check_specialized` be a macro so that the code inside (what used to be the) closure directly calls methods on the iterator types.

This also does a tiny refactor, moving the single test in `tests/fold_specialization.rs` together with the others in `tests/specializations.rs`.

Note that this may conflict with #563 (but I hope it doesn't since they touch different parts of the file)

Co-authored-by: Giacomo Stevanato <giaco.stevanato@gmail.com>
@phimuemue
Copy link
Member

Thank you.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 20, 2021

Build succeeded:

@bors bors bot merged commit b6c99ba into rust-itertools:master Aug 20, 2021
@SkiFire13 SkiFire13 deleted the process-results-fold branch August 20, 2021 18:32
@Ten0
Copy link
Contributor

Ten0 commented Aug 20, 2021

The mapper function gets a Box<dyn Iterator<Item = IterItem> + 'a> which is (almost) unspecialized anyway due to the implementation of Iterator for Box<dyn Iterator>

https://github.com/rust-lang/rust/blob/914a1e2c517dfbaa23a4ec4a3eebefb3e2c253c2/library/alloc/src/boxed.rs#L1566-L1581
Looks like it's true!
That's unexpected! Why doesn't rust specialize this? :o

Looks like we should replace the iterator dynamic dispatch in check_specialized with a macro (like the one you created), as it does not fulfill its purpose.

@scottmcm
Copy link
Contributor

That's unexpected! Why doesn't rust specialize this? :o

Because all the generic methods on Iterator can't flow into the dyn thing. So anything called on dyn Iterator can't use any of the try_fold overrides or similar.

@Ten0
Copy link
Contributor

Ten0 commented Aug 20, 2021

Had finally figured out the issues with Sized (actually, none of the methods that take self can flow in the dyn Iterator) and was coming back to edit my comment when I saw your answer from 1 minute ago ^^

Looks like we should replace the iterator dynamic dispatch in check_specialized with a macro (like the one you created), as it does not fulfill its purpose.

Actually already done in bf4ca16

@jswrenn jswrenn added this to the next milestone Sep 13, 2021
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.

process_results is lacking a lot of function specializations
5 participants