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

gaps documentation is ambiguous w/r/t empty ranges #38

Closed
acully-vmware opened this issue Jan 27, 2022 · 4 comments
Closed

gaps documentation is ambiguous w/r/t empty ranges #38

acully-vmware opened this issue Jan 27, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@acully-vmware
Copy link

The documentation says:

    /// Gets an iterator over all the maximally-sized ranges
    /// contained in `outer_range` that are not covered by
    /// any range stored in the map.

This indicates that gaps will find holes in the range-map that intersect outer_range... We had an engineer check if a range was completely covered by the RangeMap by if range_map.gaps(&check_range).next().is_none() {...}, which seems like it should work (based on the documentation), but broke in our case, because the gaps iterator can return empty ranges:

    #[test]
    fn adjacent_no_coalesce() {
        let mut range_map: RangeMap<u32, bool> = RangeMap::new();
        range_map.insert(2..5, false);
        range_map.insert(5..8, true);
        let outer_range = 2..8;
        let mut gaps = range_map.gaps(&outer_range);
        // Should yield an empty range.
        let gap = gaps.next();
        assert_eq!(gap, Some(5..5));
        assert_eq!(gap.unwrap().is_empty(), true);
    }

I think the current behavior is good, but the documentation could be improved to better indicate that gaps can return empty ranges.

@jeffparsons jeffparsons added the bug Something isn't working label Jan 27, 2022
@jeffparsons
Copy link
Owner

Thanks for reporting this, @acully-vmware. This looks like a bug to me, not just ambiguous documentation. In your example 5 is covered by the map, so I don't think that it's valid to consider 5..5 to be a gap.

I'll look into it.

@acully-vmware
Copy link
Author

Sure, but 5 isn't in 5..5, either. It can be useful to know that there's a boundary between two elements of the RangeMap at 5, though I agree it doesn't sound like a "gap"... I can go either way, on this one.

@jeffparsons
Copy link
Owner

I agree that finding the all the boundaries between ranges could be useful, but I'd rather introduce a separate iterator for that. The current behavior in the gaps iterator was an oversight, and I think most people would expect it to behave as your colleague did.

@jeffparsons
Copy link
Owner

Fixed here: dfdf5b0

New release 1.0.1 with the fix is now available on crates.io.

Thanks for the report! 💖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants