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 Bitfield cut operator and other improvements #617

Merged
merged 4 commits into from
Aug 17, 2020
Merged

Conversation

timvermeulen
Copy link
Contributor

This PR adds revamped bitfield combinator logic, which was necessary for the cut operation, and the other combinators (union / intersection / difference) benefit from it as well. Mainly:

  • a Combinator trait that lets you concisely specify how two bitfields should be combined
  • a Combine range iterator that actually combines them according to the specified Combinator

This logic (+ explanations) can all be found in the combine.rs file. I also threw in the symmetric difference operation (usable with the ^ and ^= operators) because of how easy it was with the new setup.

The benchmarks didn't show a noticeable difference compared to before these changes.

Other changes:

  • Removed the BitField::{merge, intersection, difference} methods because they returned range iterators and we didn't use them, the corresponding operators still work like before
  • Renamed the only internally used merge method to union in order to avoid confusion with the inernal Merge type in combine.rs that is subtly different
  • Created a new bitfield/iter module, so the diff is a bit larger than the actual changes

Closes #602

Comment on lines 391 to 398
impl<A, B, C> RangeIterator for _Combine<A, B, C>
where
A: RangeIterator,
B: RangeIterator,
C: Combinator,
{
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
impl<A, B, C> RangeIterator for _Combine<A, B, C>
where
A: RangeIterator,
B: RangeIterator,
C: Combinator,
{
}

You explicitly say this is not satisfied. (I'm still a bit confused about why _Combine is even necessary) You are wrapping it as Combine(Merge(_Combine)) but does this just remove some code duplication or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, great catch, that is indeed not satisfied. Combine does implement it, _Combine doesn't.

The _Combine layering is less than ideal, if trait methods could return impl Trait then there'd be no need for a type that wraps Merge<_Combine>. I found that integrating the Merge code into what is now _Combine made it a lot messier, and Merge will indeed be able to be used elsewhere in the future (in unioning multiple bitfields, which is currently implemented very naively).

@timvermeulen timvermeulen merged commit 336ae3b into main Aug 17, 2020
@timvermeulen timvermeulen deleted the tim/bitfield-cut branch August 17, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the BitField cut operation
3 participants