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

Add iterator versions of BTreeSet operations #556

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DevinR528
Copy link

The motivation for this is when using union, intersection, difference, and symmetric_difference the items have to be cloned, this avoids that and can return borrowed items. What I was shooting for was as if the signature to BTreeSet::intersection was

pub fn difference<'a>(&'a self, other: &'a BTreeSet<Q>) -> Difference<'a, Q>
where
    T: Borrow<Q>,
    Q: Ord,
{...}

The code is taken from the std lib and altered to work with iterators so it should be sound, still working out a few kinks but nothing major.

I did try implementing the above signature in the std lib but was unable (could be just me) so I tried this out and got it working! Let me know if this is helpful as part of itertools.

Thanks by the way for a great crate!

Huge problem

This relies on the iterator returning items in sorted order, is this an acceptable invariant to only document (is there any way to add a trait to signify that the iterator needs to be sorted?).

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented May 15, 2024

@DevinR528 This is quite some work you done here!

This is huge (even without the now unneeded first commit): small PRs are more likely to be considered/reviewed/merged. So as is, it will likely be closed.
There is no SortedIterator trait and it seems an uncommon requirement to me, it seems contrived to work with iterators, but it could be documented.
Maybe a simpler version of those could wrap MergeJoinBy and roughly-filter_map the elements.

But I wonder if other people would be interested in this, and this is the major block point to me here.
In which use cases would it be useful?

EDIT: That being said, I found a related issue #695.

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.

2 participants