Refactorfor for less allocation.#13
Conversation
|
There may be some redundant if branches. I'm considering whether the first item special case could be dropped. |
AureliaDolo
changed the title
|
I don't think I can do the same for sub_interval_unchecked as the interval to subtract may span across all elements in self.intervals |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Looks good to me ✨ mostly nits and stylistic comments below. I think the logic is sane (one way to double-check that would be to have fun with a small fuzzer for this function :D). It might be nice to add a doc comment for this function expliciting the invariant that the new interval won't overlap with any existing interval.
| if idx == self.intervals.len() { | ||
| // previously treated | ||
| unreachable!() | ||
| } |
There was a problem hiding this comment.
nit: unnecessary because the .get().unwrap() next line will panic the same way
| unreachable!() | ||
| } | ||
| // we can unwrap as the case interval is after the last element is treated before | ||
| let next = self.intervals.get(idx).unwrap(); |
There was a problem hiding this comment.
nit: could be simplified as &mut self.intervals[idx]
| /// Add an interval but does not check assume interval is correctly formed | ||
| fn add_interval_unchecked(&mut self, interval: &(u64, u64)) { | ||
| let mut interval = *interval; | ||
| let interval = *interval; |
There was a problem hiding this comment.
Consider adding a debug_assert! that no intervals in self.intervals overlap with the one to add, since it's an undocumented invariant?
| // it may merge with next | ||
|
|
||
| let before = self.intervals[idx]; | ||
| let after = self.intervals[idx + 1]; |
There was a problem hiding this comment.
Maybe comment that there is a next interval all the time (otherwise we would have fallen under another branch under the if let Some() = last part above)
| self.intervals.push(interval); | ||
| return; | ||
| } | ||
| if let Some(&first) = self.intervals.first() { |
There was a problem hiding this comment.
nit: you can use first_mut here (and remove the & in front of &first)...
| // unwrapping is ok as intervals is not empty | ||
|
|
||
| let last = self.intervals.last_mut().unwrap(); | ||
| *last = (last.0, interval.1); |
| new.push(interval); | ||
| new.sort(); | ||
| self.intervals = new; | ||
| if let Some(&last) = self.intervals.last() { |
| let next = self.intervals.get(idx).unwrap(); | ||
| // interval merges with next | ||
| if next.0 == interval.1 { | ||
| *self.intervals.get_mut(idx).unwrap() = (interval.0, next.1); |
There was a problem hiding this comment.
(and no need to take get_mut here with the above suggestion, and we're only modifying the left bound here)
|
|
||
| // interval merges with before and after | ||
| if interval.0 == before.1 && interval.1 == after.0 { | ||
| *self.intervals.get_mut(idx).unwrap() = (before.0, after.1); |
There was a problem hiding this comment.
Worth trying to use &mut self.intervals[idx] for definition of before above, and modify it in place here.
| } else if | ||
| // interval merges with after | ||
| interval.1 == after.0 { | ||
| unreachable!("should have been treated in the error branch before"); | ||
| // *self.intervals.get_mut(idx + 1).unwrap() = (interval.0, after.1); | ||
| } else { | ||
| // interval does not merge | ||
| unreachable!("should have been treated in the error branch before"); | ||
| // self.intervals.insert(idx + 1, interval) | ||
| } |
There was a problem hiding this comment.
nit: please remove commented code + maybe merge those two elses, since they share the same panic message?
AureliaDolo
force-pushed
the
no_alloc
branch
3 times, most recently
from
e0cec53 to
5ded370
Compare
2 participants
Fix #7