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

implement range based iteration #297

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Dr-Emann
Copy link
Member

@Dr-Emann Dr-Emann commented Nov 11, 2024

Implement bitmap.iter_range() and bitmap.into_iter_range(), based on advance_to() and advance_back_to(). Closes #290.

Open questions:

  • The equivalent to .iter_range for BTreeSet is called .range() (and so does Add range-based iteration #290). Is that a better name: .range/.iter_range? We also need an owning version, into_range vs into_iter_range? Using the names range/into_range.
  • .range on BTreeSet panics on start > end. (or start == end when both are excluded). Should we? Decided to panic on ranges to match BTreeSet. The code is less pretty having to differentiate between validly empty ranges vs invalid ranges, but probably worth it.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @Dr-Emann,

I see that this PR is mostly the same as #298. Can you rebase on main once #299 is merged, please?

However, you propose interesting open questions that I will try to answer here.

The equivalent to .iter_range for BTreeSet is called .range() (and so does #290). Is that a better name: .range/.iter_range? We also need an owning version, into_range vs into_iter_range?

I indeed prefer that we use the range naming instead of the iter_range. This way, as you propose, we will be closer to the standard naming. I also think that, at some point, we will want to support extracting ranges but we will probably name this method extract_range.

.range on BTreeSet panics on start > end. (or start == end when both are excluded). Should we?

Yes, I think a range with a start > end is not expected and most likely an error. We should panic. Same for the start == end case.

@Dr-Emann
Copy link
Member Author

Hmm, the current utility to to an inclusive range can't currently differentiate 1..1 (an empty range I assume we want to allow) from 1..=0 (an empty range I assume we want to disallow.

@Kerollmops
Copy link
Member

the current utility to to an inclusive range can't currently differentiate 1..1 (an empty range I assume we want to allow) from 1..=0 (an empty range I assume we want to disallow.

Can we do it by hand then? If we cannot then, I think, it's better to just accept them as we don't want to panic for valid empty ranges.

@Dr-Emann
Copy link
Member Author

@Kerollmops Looks like another dependency update broke the MSRV.

@Kerollmops
Copy link
Member

Kerollmops commented Nov 25, 2024

Hey @Dr-Emann 👋

Looks like another dependency update broke the MSRV.

Would you mind rebasing on main when #289 land? I bumped the MSRV to 1.71.1 (again).

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Whoops, still some formating issue 😞

Implement `bitmap.iter_range()` and `bitmap.into_iter_range()`, based on
`advance_to()` and `advance_back_to()`.

Co-authored-by: Matthew Herzl <matthew@herzl.email>
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Looks perfect to me, thank you very much for the hard work and patience 🐢

/// Convert a `RangeBounds<u32>` object to `RangeInclusive<u32>`,
pub fn convert_range_to_inclusive<R>(range: R) -> Option<RangeInclusive<u32>>
pub fn convert_range_to_inclusive<R>(range: R) -> Result<RangeInclusive<u32>, ConvertRangeError>
Copy link
Member

Choose a reason for hiding this comment

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

Indeed the code is not pretty but it is very useful, thank you 😊

@Kerollmops Kerollmops merged commit d491df6 into RoaringBitmap:main Nov 27, 2024
4 checks passed
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