Possible fix for #3: bounds check in heap_sort::sink#6
Possible fix for #3: bounds check in heap_sort::sink#6Rohan-GRH wants to merge 1 commit intoAbrarNitk:masterfrom
Conversation
Change j < n to j + 1 < n to prevent out-of-bounds access on arr[j + 1] when j == n - 1. Fixes AbrarNitk#3
There was a problem hiding this comment.
Pull request overview
This PR aims to address Issue #3 by adjusting the bounds check in heap_sort::sink to prevent potential out-of-bounds access when comparing the right child.
Changes:
- Updated the right-child existence guard in
sinkfromj < ntoj + 1 < n.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while 2 * k < n { | ||
| let mut j = 2 * k + 1; | ||
| if j < n && p(&arr[j], &arr[j + 1]) { | ||
| if j + 1 < n && p(&arr[j], &arr[j + 1]) { |
There was a problem hiding this comment.
The n parameter is currently used as an inclusive last index (e.g., sort_by passes arr.len() - 1 and decrements l before calling sink). With that contract, the right-child-exists check should be j < n (so j + 1 <= n is in-bounds). Changing this to j + 1 < n skips the right child when it is exactly at index n (e.g., n=6, k=2 => left=5 right=6), which can break the heap property and lead to incorrect sorting. Either revert this condition back to j < n, or change sink’s contract to treat n as an exclusive upper bound/length and update all call sites (sort_by, loop over l, and tests) + add a regression test for the boundary case where right child is at the last index.
| if j + 1 < n && p(&arr[j], &arr[j + 1]) { | |
| if j < n && p(&arr[j], &arr[j + 1]) { |
Hi, thank you for this great library!
I see issue #3 was closed without comment. If there's a reason this change is not desired, I'd really appreciate understanding why so I can learn. Thanks!
I came across a potential issue in the
sinkfunction inheap_sort.rs(as described in #3). The bounds checkj < non line 42 ensuresarr[j]is valid, but may not preventarr[j + 1]from going out of bounds whenj == n - 1.I've made a small change (
j < n→j + 1 < n) that I believe could address this. Of course, I may be missing some context — please feel free to let me know if this is intentional or if you'd prefer a different approach.Thanks for your time!