sort: move legacy sort! recursion into private kernels - #63228
Conversation
LilithHafner
left a comment
LilithHafner
left a comment
There was a problem hiding this comment.
_quicksort! and _small_sort! are weirdly asymmetric.
|
Could you elaborate? |
|
The presence of the algorithm positional arg in _mergesort! but not _smal_lsort! is odd, the only assymmetry I would expect there is for partial quicksort, and even then I'd rather see full symmetry (i.e. all pass algo). But as I look more, I think it would be better for _quicksort! and friends to call the modern API directly rather than making a new stub that does that. Eliminating the inconsistency by eliminating the function altogether. |
|
I'm fairly neutral on calling modern everywhere vs having a single point of translation, but if we do use a single point of translation it should be consistent with the functions that serve a similar role and include the (unused) alg arg, or all of them should not and partialquicksort should pass a bare int. |
The `sort!(v, lo, hi, alg, order)` methods for `QuickSort`, `MergeSort` and `PartialQuickSort` recursed into themselves, so the `checkbounds` added in #63216 ran on every recursive call and the public methods still contained the `@inbounds` recursion. Split each into a public wrapper that checks `lo:hi` once and a private recursive kernel (`_quicksort!`, `_mergesort!`, `_partialquicksort!`) that assumes the indices are valid. The small base case calls `_sort!` with `InsertionSort` directly instead of going back through the checked public entry point. Assisted-by: Claude Code (Fable 5.1)
27c5594 to
4807a28
Compare
|
Better now? |
2 participants
Avoids
sort!having memory-unsafe behavior while at the same time avoiding the bounds check in every recursive call (cf #63216 (comment))Assisted-by: Claude Code (Fable 5.1)