Add support to merge sort with a limit #222
Conversation
sundy-li
changed the title
Codecov Report
@@ Coverage Diff @@
## main #222 +/- ##
==========================================
- Coverage 76.94% 76.84% -0.10%
==========================================
Files 229 229
Lines 19536 19610 +74
==========================================
+ Hits 15031 15070 +39
- Misses 4505 4540 +35
Continue to review full report at Codecov.
|
src/compute/merge_sort/mod.rs
Outdated
| for (index, start, len) in slices { | ||
| growable.extend(index, start, len) | ||
| if len + current_len >= limit { |
There was a problem hiding this comment.
I think this probably makes the code without a limit (limit=None) slower.
There was a problem hiding this comment.
This can be avoided by having two different implementations/bodies.
There was a problem hiding this comment.
I would prefer to not expose a new function just for this; wouldn't it make sense to move this if to outside of the loop and keep the function with the limit argument?
There was a problem hiding this comment.
I'm afraid the if can't move outside of the loop because the slice is an iterator.
There was a problem hiding this comment.
If you do slices.into_iter() in two code blocks instead of reusing it, it shouldn't be a compile error?
There was a problem hiding this comment.
What I mean is thatslices is lazy iterated, we don't know the item of len unless we call next(), so the if check must be inside the loop when limit is some value.
There was a problem hiding this comment.
I was thinking in something like this:
...
if let Some(limit) = limit {
iterate with the limit condition
} else {
iterate without the limit condition
}
...
growable.to_box()
jorgecarleitao
added
backwards-incompatible
enhancement
jorgecarleitao
changed the title
jorgecarleitao
changed the title
Closes #221
Backward incompatible changes
The functions
compute::merge_sort::take_arraysandcompute::merge_sort::merge_sortnow expects a third argument,Option<usize>, denoting an optional limit. To migrate, addNoneto their calls.