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

Port get_index and the bitset functions #107

Merged
merged 3 commits into from
Jun 20, 2023

Conversation

Dr-Emann
Copy link
Member

  • Port roaring_bitmap_get_index as Bitmap::position (the name was chosen to match with Iterator::position)
  • Update bindgen to add bindings for bitset_* methods/types/variables
  • Port bitset_* functions, and roaring_bitmap_to_bitset as Bitmap::to_bitset
  • Update to version 1.3.0 of CRoaring

Because of the version update, this will clash with #105 and #106

`Bitmap::position` is implmemented in terms of the new
`roaring_bitmap_get_index` function. The name `position` was chosen
because it mirrors the rust `Iterator::position` function, which does
the same thing.
CRoaring introduced a `bitset` type, and the abilty to convert a roaring
bitmap into a dense bitset. This adds wrappers around the new bitset
functions
#[inline]
#[doc(alias = "bitset_clear")]
#[doc(alias = "bitset_fill")]
pub fn fill(&mut self, value: bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I like this. Any reason why bitset_clear and bitset_fill aren't separate?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thoughts were to be closer to <[bool]>::fill, and that since it's inline, .fill(constant) will be optimized to a single call without a branch anyway.

I really don't want to use the name clear (since it's too similar to Vec::clear but does something VERY different), so the alternative in my mind would be Bitset::fill_zeros and Bitset::fill_ones.

I'm not hard set on this option, but it seemed like the best option to me.

@saulius
Copy link
Member

saulius commented Jun 20, 2023

Really struggling to find proper time to review larger amounts of code lately, but generally it looks great to me! Thanks one more time for the contribution.

On that note, @Dr-Emann, you made some great contributions over the last years to croaring-rs. Would you be willing to maintain the project together? I have an intention to transfer it over to RoaringBitmap Github org, if @lemire is up for it.

@saulius saulius merged commit 46b6d62 into RoaringBitmap:master Jun 20, 2023
12 checks passed
@andreas
Copy link
Contributor

andreas commented Jun 20, 2023

I'm currently advocating for the croaring-crate to be supported by AWS RDS for Postgres via plrust (blog post). Having croaring-rs adopted by the RoaringBitmap org would be a big benefit in that regard, so selfishly I'd love to see that happen 😊

@Dr-Emann Dr-Emann deleted the new_funcs_from_upstream branch June 20, 2023 13:10
@Dr-Emann
Copy link
Member Author

I'd certainly be willing to help maintain the project, and I also personally think it would make sense to end up under the RoaringBitmap org.

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.

None yet

3 participants