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

Insert/remove range with bounds #115

Merged
merged 6 commits into from
Oct 18, 2021
Merged

Insert/remove range with bounds #115

merged 6 commits into from
Oct 18, 2021

Conversation

oliverdding
Copy link
Contributor

@oliverdding oliverdding commented Sep 23, 2021

Implementation of insert_range and remove_range functions that accept the RangeBounds trait instead of a Range struct.

This PR fix #5 and #113.

Done:

  • Switch inner parameters to RangeBounds
    • insert_range
    • remove_range
  • Fix boundary condition in insert_range/remove_range
  • Add more test cases
  • switching to RangeInclude on the Container and Store
    • insert_range
    • remove_range

src/bitmap/container.rs Show resolved Hide resolved
src/bitmap/util.rs Outdated Show resolved Hide resolved
src/bitmap/util.rs Outdated Show resolved Hide resolved
src/bitmap/util.rs Outdated Show resolved Hide resolved
src/bitmap/util.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/store.rs Show resolved Hide resolved
src/bitmap/store.rs Show resolved Hide resolved
src/bitmap/store.rs Show resolved Hide resolved
src/treemap/util.rs Outdated Show resolved Hide resolved
@oliverdding
Copy link
Contributor Author

Hi @Kerollmops

Thanks for your reviewing!

Recently I might have no time to code since I'm on my way to travel 👻. But I would come back ASAP.

@oliverdding
Copy link
Contributor Author

Hi @Kerollmops
Sorry for late.
I've marked the conversation resolved if I accepted your advice or it's duplicate, leaving some topics to talk about.

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 very much for your changes, can you please change your code a little bit, please?
I think I will merge your code just after :)

src/treemap/inherent.rs Outdated Show resolved Hide resolved
src/bitmap/inherent.rs Show resolved Hide resolved
src/bitmap/inherent.rs Outdated Show resolved Hide resolved
@oliverdding
Copy link
Contributor Author

Hi @Kerollmops
I have finished all the jobs here ;)

And I just want to mention the util::convert_range_to_inclusive, which is done like this:

pub fn convert_range_to_inclusive<R>(range: R) -> Option<RangeInclusive<u32>>
where
    R: RangeBounds<u32>,
{
    let start: u32 = match range.start_bound() {
        Bound::Included(&i) => i,
        Bound::Excluded(&u32::MAX) => return None, // must exit here
        Bound::Excluded(&i) => i + 1,
        Bound::Unbounded => 0,
    };
    let end: u32 = match range.end_bound() {
        Bound::Included(&i) => i,
        Bound::Excluded(&0) => return None, // must exit here
        Bound::Excluded(&i) => i - 1,
        Bound::Unbounded => u32::MAX,
    };
    if end < start {
        return None;
    }
    Some(start..=end)
}

In this way we can idiomatically avoid the saturating_sub with simplier logic, and I think you would like 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.

It is indeed a lot better @oliverdding, I love what you've done here.
I am merging all of that right now!

@Kerollmops Kerollmops merged commit 14376a0 into RoaringBitmap:insert-remove-range-bounds Oct 18, 2021
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.

The insert_range method does not properly handle boundary condition Implement Bounded Iterator
2 participants