Skip to content
New issue

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

#

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

Already on GitHub? # to your account

In partial_insertion_sort, moved the SHORTEST_SHIFTING check outside of loop #1226

Closed
wants to merge 1 commit into from
Closed

In partial_insertion_sort, moved the SHORTEST_SHIFTING check outside of loop #1226

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/slice/sort.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,11 @@ where
let len = v.len();
let mut i = 1;

// Don't shift elements on short arrays, that has a performance cost.
if len < SHORTEST_SHIFTING {
return false;
}

for _ in 0..MAX_STEPS {
// SAFETY: We already explicitly did the bound checking with `i < len`.
// All our subsequent indexing is only in the range `0 <= index < len`
Expand All @@ -238,11 +243,6 @@ where
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the SHORTEST_SHIFTING condition is moved up, it changes this case that would have returned true. I haven't fully analyzed that effect, but it's not just about "readability" at that point.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right. The behaviour on short sorted slices changes, my bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then I'll close this - thanks anyway!

}

// Don't shift elements on short arrays, that has a performance cost.
if len < SHORTEST_SHIFTING {
return false;
}

// Swap the found pair of elements. This puts them in correct order.
v.swap(i - 1, i);

Expand Down