Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core::slice::sort::partial_insertion_sort: Redundant call to insertion_sort_shift_right #125618

Open
hvenev-insait opened this issue May 27, 2024 · 2 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@hvenev-insait
Copy link

In partial_insertion_sort, an invariant is maintained that v[..i] is sorted. When a non-sorted pair v[i-1] > v[i] is encountered, they are swapped and the last element of v[..i] is shifted to the left into its correct spot. Then for some reason insertion_sort_shift_right is called on v[..i], attempting to shift v[0] to the right, which never does anything.

Before commit a3065a1, the shift was happening on v[i..] (instead of v[..i]), attempting to shift v[i] to the right, which makes more sense.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 27, 2024
@workingjubilee
Copy link
Contributor

have you considered opening a PR?

@hvenev-insait
Copy link
Author

Not yet. I am not sure what the best way to fix this is. Do we want to maintain the current behavior (in which case all we need to do is delete insertion_sort_shift_right), or do we want to go back to the behavior before a3065a1 (probably call insertion_sort_shift_right on v[i..] instead of v[..i])?

Also, do we have any benchmarks for that code?

@jieyouxu jieyouxu added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants