Conversation
ia7ck
force-pushed
the
disjoint-intervals
branch
from
2f54902 to
c8ce624
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Rust library for managing disjoint (non-overlapping) intervals with support for insertion, removal, and querying operations. The library uses a BTreeMap internally to maintain intervals in sorted order and provides a fold-based API for tracking changes during operations.
Key changes:
- Implements
DisjointIntervals<T>struct with insert/remove operations that automatically merge overlapping intervals - Provides query methods (
ge,le) to find intervals relative to a given point - Includes comprehensive test coverage for insert and remove operations with fold tracking
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| libs/disjoint_intervals/Cargo.toml | Package manifest defining library metadata and configuration |
| libs/disjoint_intervals/src/lib.rs | Core implementation of disjoint interval data structure with insert, remove, and query operations plus test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn len(&self) -> usize { | ||
| self.intervals.len() | ||
| } |
There was a problem hiding this comment.
The len method lacks unit test coverage. While the method is simple, adding a test would help ensure it behaves correctly, especially after insert and remove operations. Consider adding tests that verify len() returns the correct number of disjoint intervals after various operations.
| pub fn is_empty(&self) -> bool { | ||
| self.intervals.is_empty() | ||
| } |
There was a problem hiding this comment.
The is_empty method lacks unit test coverage. While the method is simple, adding a test would help ensure it behaves correctly. Consider adding tests that verify is_empty() returns true for a new instance and false after inserting intervals.
| pub fn ge(&self, x: T) -> Option<Range<T>> { | ||
| self.intervals | ||
| .range(x..) | ||
| .next() | ||
| .map(|(&start, &end)| start..end) | ||
| } |
There was a problem hiding this comment.
The ge method lacks unit test coverage beyond its documentation example. Since other methods in this module have dedicated unit tests, consider adding tests that cover various edge cases such as searching in empty intervals, searching for values before/after all intervals, and searching for values that fall within existing intervals.
| pub fn le(&self, x: T) -> Option<Range<T>> { | ||
| self.intervals | ||
| .range(..=x) | ||
| .last() | ||
| .map(|(&start, &end)| start..end) | ||
| } |
There was a problem hiding this comment.
The le method lacks unit test coverage beyond its documentation example. Since other methods in this module have dedicated unit tests, consider adding tests that cover various edge cases such as searching in empty intervals, searching for values before/after all intervals, and searching for values that fall within existing intervals.
libs/disjoint_intervals/src/lib.rs
Outdated
|
|
||
| /// 区間を追加する | ||
| /// | ||
| /// 初期値 `init,` 関数 `f` による畳み込み結果を返す |
There was a problem hiding this comment.
The documentation comment has a trailing comma after "init" which appears to be a typo. It should read "初期値 init 関数 f による畳み込み結果を返す" (without the comma after init).
| /// 初期値 `init,` 関数 `f` による畳み込み結果を返す | |
| /// 初期値 `init` 関数 `f` による畳み込み結果を返す |
libs/disjoint_intervals/src/lib.rs
Outdated
|
|
||
| /// 区間を削除する | ||
| /// | ||
| /// 初期値 `init,` 関数 `f` による畳み込み結果を返す |
There was a problem hiding this comment.
The documentation comment has a trailing comma after "init" which appears to be a typo. It should read "初期値 init 関数 f による畳み込み結果を返す" (without the comma after init).
libs/disjoint_intervals/src/lib.rs
Outdated
| match self.intervals.range(..=interval.start).last() { | ||
| Some((&prev_start, &prev_end)) if interval.start <= prev_end => { | ||
| if interval.start < prev_end { | ||
| acc = f(acc, InsertItem::Overlap(interval.start..prev_end)); |
There was a problem hiding this comment.
There's a logical issue in the overlap calculation. When interval.start < prev_end, the overlap range is reported as interval.start..prev_end, but this doesn't account for when interval.end might be less than prev_end. In that case, the actual overlap should be interval.start..interval.end, not interval.start..prev_end. This could lead to incorrect overlap reporting when inserting an interval that is completely contained within an existing interval.
| acc = f(acc, InsertItem::Overlap(interval.start..prev_end)); | |
| let overlap_end = if prev_end < interval.end { prev_end } else { interval.end }; | |
| acc = f(acc, InsertItem::Overlap(interval.start..overlap_end)); |
1 participant
No description provided.