refactor: replace Range with a bounded implementation #112
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ | |
| use crate::{internal::small_vec::SmallVec, version_set::VersionSet}; | ||
| use std::ops::RangeBounds; | ||
| use std::{ | ||
| fmt::{Debug, Display, Formatter}, | ||
| ops::Bound::{self, Excluded, Included, Unbounded}, | ||
| }; | ||
|
|
@@ -202,15 +201,40 @@ impl<V: Ord> Range<V> { | |
| Excluded(v) => Excluded(v.clone().into()), | ||
| Unbounded => Unbounded, | ||
| }; | ||
| if valid_segment(&start, &end) { | ||
| Self { | ||
| segments: SmallVec::one((start, end)), | ||
| } | ||
| } else { | ||
| Self::empty() | ||
| } | ||
| } | ||
|
|
||
| fn check_invariants(self) -> Self { | ||
| if cfg!(debug_assertions) { | ||
| for (i, (s, e)) in self.segments.iter().enumerate() { | ||
| if matches!(s, Unbounded) && i != 0 { | ||
| panic!() | ||
| } | ||
| if matches!(e, Unbounded) && i != (self.segments.len() - 1) { | ||
| panic!() | ||
| } | ||
| } | ||
| for p in self.segments.as_slice().windows(2) { | ||
| match (&p[0].1, &p[1].0) { | ||
| (Included(l_end), Included(r_start)) => assert!(l_end < r_start), | ||
| (Included(l_end), Excluded(r_start)) => assert!(l_end < r_start), | ||
| (Excluded(l_end), Included(r_start)) => assert!(l_end < r_start), | ||
| (Excluded(l_end), Excluded(r_start)) => assert!(l_end <= r_start), | ||
| (_, Unbounded) => panic!(), | ||
| (Unbounded, _) => panic!(), | ||
| } | ||
| } | ||
| for (s, e) in self.segments.iter() { | ||
| assert!(valid_segment(s, e)); | ||
| } | ||
| } | ||
| self | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -223,142 +247,62 @@ fn bound_as_ref<V>(bound: &Bound<V>) -> Bound<&V> { | |
| } | ||
| } | ||
|
|
||
| fn valid_segment<T: PartialOrd>(start: &Bound<T>, end: &Bound<T>) -> bool { | ||
| match (start, end) { | ||
| (Included(s), Included(e)) => s <= e, | ||
| (Included(s), Excluded(e)) => s < e, | ||
| (Excluded(s), Included(e)) => s < e, | ||
| (Excluded(s), Excluded(e)) => s < e, | ||
| (Unbounded, _) | (_, Unbounded) => true, | ||
| } | ||
| } | ||
|
|
||
| impl<V: Ord + Clone> Range<V> { | ||
| /// Computes the union of this `Range` and another. | ||
| pub fn union(&self, other: &Self) -> Self { | ||
| self.complement() | ||
| .intersection(&other.complement()) | ||
| .complement() | ||
| .check_invariants() | ||
| } | ||
|
|
||
| /// Computes the intersection of two sets of versions. | ||
| pub fn intersection(&self, other: &Self) -> Self { | ||
|
There was a problem hiding this comment. I spent time today trying to write a "more obviously correct" fn check_invariants(&self) {
if cfg!(debug_assertions) {
for (i, (s, e)) in self.segments.iter().enumerate() {
if matches!(s, Unbounded) && i != 0 {
panic!()
}
if matches!(e, Unbounded) && i != (self.segments.len() - 1) {
panic!()
}
}
for p in self.segments.as_slice().windows(2) {
match (&p[0].1, &p[1].0) {
(Included(l_end), Included(r_start)) => assert!(l_end < r_start),
(Included(l_end), Excluded(r_start)) => assert!(l_end < r_start),
(Excluded(l_end), Included(r_start)) => assert!(l_end < r_start),
(Excluded(l_end), Excluded(r_start)) => assert!(l_end <= r_start),
(_, Unbounded) => panic!(),
(Unbounded, _) => panic!(),
}
}
for (s, e) in self.segments.iter() {
assert!(match (s, e) {
(Included(s), Included(e)) => s <= e,
(Included(s), Excluded(e)) => s < e,
(Excluded(s), Included(e)) => s < e,
(Excluded(s), Excluded(e)) => s < e,
(Unbounded, _) | (_, Unbounded) => true,
});
}
}
}Perhaps we can add a call to it to the end of all methods that construct a There was a problem hiding this comment. This is as close as I came: perf is slightly worse, but I find it more readable. what do you think? There was a problem hiding this comment. I do like that its a lot shorter and easier to follow, my code had a lot of cases. How much worse is the performance? There was a problem hiding this comment. Rerunning the benchmarks now I am not seeing significant differences between our two implementations! I guess I shouldn't try benchmarking at 2 o'clock in the morning. There was a problem hiding this comment. Haha, Ill copy your implementation into the MR. Should I also add the invariant check? There was a problem hiding this comment. Or can I simply merge your changes? There was a problem hiding this comment. I like the check invariants function! Is the first There was a problem hiding this comment. I cherry-picked your code (crediting you) and added the suggestion about |
||
| let mut segments: SmallVec<Interval<V>> = SmallVec::empty(); | ||
| let mut left_iter = self.segments.iter().peekable(); | ||
| let mut right_iter = other.segments.iter().peekable(); | ||
|
|
||
| while let (Some((left_start, left_end)), Some((right_start, right_end))) = | ||
| (left_iter.peek(), right_iter.peek()) | ||
| { | ||
| let start = match (left_start, right_start) { | ||
| (Included(l), Included(r)) => Included(std::cmp::max(l, r)), | ||
| (Excluded(l), Excluded(r)) => Excluded(std::cmp::max(l, r)), | ||
|
|
||
| (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i <= e => Excluded(e), | ||
| (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e < i => Included(i), | ||
| (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), | ||
| _ => unreachable!(), | ||
| } | ||
| .cloned(); | ||
| let end = match (left_end, right_end) { | ||
| (Included(l), Included(r)) => Included(std::cmp::min(l, r)), | ||
| (Excluded(l), Excluded(r)) => Excluded(std::cmp::min(l, r)), | ||
|
|
||
| (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i >= e => Excluded(e), | ||
| (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e > i => Included(i), | ||
| (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), | ||
| _ => unreachable!(), | ||
| } | ||
| .cloned(); | ||
| left_iter.next_if(|(_, e)| e == &end); | ||
| right_iter.next_if(|(_, e)| e == &end); | ||
| if valid_segment(&start, &end) { | ||
| segments.push((start, end)) | ||
| } | ||
| } | ||
|
|
||
| Self { segments }.check_invariants() | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -614,7 +558,7 @@ pub mod tests { | |
|
|
||
| #[test] | ||
| fn from_range_bounds(range in any::<(Bound<u32>, Bound<u32>)>(), version in version_strat()) { | ||
| let rv: Range<_> = Range::from_range_bounds(range); | ||
| assert_eq!(range.contains(&version), rv.contains(&version)); | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there might be representations special cases where we have an invalid segment that cannot be checked by this function like
(Unbounded, Excluded(0))in the case where versions areu32. It depends if there are other ways to represent the left-most and right-most bounds with the version type.What do you guys think of that? negligible?