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

Override default fold and rfold implementations for the iterators #277

Closed
oskgo opened this issue May 25, 2024 · 2 comments · Fixed by #280
Closed

Override default fold and rfold implementations for the iterators #277

oskgo opened this issue May 25, 2024 · 2 comments · Fixed by #280

Comments

@oskgo
Copy link
Contributor

oskgo commented May 25, 2024

fold and rfold are APIs for internal iteration, which is sometimes more efficient than external iteration (next and next_back). It is always possible to implement internal iteration in terms of external, which is what the default impl does, but this implementation need not be optimal. Flatten (which roaring uses) has more efficient fold and rfold implementations than the default. Roaring should use the implementations from Flatten rather than the default implementations.

Ideally you would implement try_fold and try_rfold as well, but they cannot be implemented until the Try trait is stabilized (rust-lang/rust#84277).

This might apply to other default implementations overridden by Flatten as well.

@Kerollmops
Copy link
Member

Hey @oskgo 👋

Thank you for the information and proposal. It is very interesting. Would you mind doing a PR or something at some point? Or is it more like a tracking issue?

@oskgo
Copy link
Contributor Author

oskgo commented May 28, 2024

You seem to be fine with the change I'm proposing, so I can write up a PR.

It might make sense to have a tracking issue if it can be set up to be notified when Try stabilizes, but I don't know how to set that up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants