-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add slice::sort_by_cached_key as a memoised sort_by_key #48639
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
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
ea6a1bd
Compute each key only one during slice::sort_by_key
varkor 670e69e
Update documentation for sort_by_key
varkor b8452cc
Clarify behaviour of sort_unstable_by_key with respect to sort_by_key
varkor 9fbee35
Add a test for sort_by_key
varkor 21fde09
Update documentation
varkor 7dcfc07
Cull the quadratic
varkor bdcc6f9
Index enumeration by minimally sized type
varkor f41a26f
Add sort_by_cached_key method
varkor b430cba
Fix use of unstable feature in test
varkor ca3bed0
Improve and fix documentation for sort_by_cached_key
varkor 9896b38
Clarify time complexity
varkor 81edd17
Check that the size optimisation is not redundant
varkor 785e3c3
Add lexicographic sorting benchmark
varkor eca1e18
Add stability test for sort_by_cached_key
varkor 9c7b69e
Remove mentions of unstable sort_by_cached key from stable documentation
varkor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -425,6 +425,14 @@ fn test_sort() { | |
v.sort_by(|a, b| b.cmp(a)); | ||
assert!(v.windows(2).all(|w| w[0] >= w[1])); | ||
|
||
// Sort in lexicographic order. | ||
let mut v1 = orig.clone(); | ||
let mut v2 = orig.clone(); | ||
v1.sort_by_key(|x| x.to_string()); | ||
v2.sort_by_cached_key(|x| x.to_string()); | ||
assert!(v1.windows(2).all(|w| w[0].to_string() <= w[1].to_string())); | ||
assert!(v1 == v2); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! |
||
// Sort with many pre-sorted runs. | ||
let mut v = orig.clone(); | ||
v.sort(); | ||
|
@@ -477,24 +485,29 @@ fn test_sort_stability() { | |
// the second item represents which occurrence of that | ||
// number this element is, i.e. the second elements | ||
// will occur in sorted order. | ||
let mut v: Vec<_> = (0..len) | ||
let mut orig: Vec<_> = (0..len) | ||
.map(|_| { | ||
let n = thread_rng().gen::<usize>() % 10; | ||
counts[n] += 1; | ||
(n, counts[n]) | ||
}) | ||
.collect(); | ||
|
||
// only sort on the first element, so an unstable sort | ||
let mut v = orig.clone(); | ||
// Only sort on the first element, so an unstable sort | ||
// may mix up the counts. | ||
v.sort_by(|&(a, _), &(b, _)| a.cmp(&b)); | ||
|
||
// this comparison includes the count (the second item | ||
// This comparison includes the count (the second item | ||
// of the tuple), so elements with equal first items | ||
// will need to be ordered with increasing | ||
// counts... i.e. exactly asserting that this sort is | ||
// stable. | ||
assert!(v.windows(2).all(|w| w[0] <= w[1])); | ||
|
||
let mut v = orig.clone(); | ||
v.sort_by_cached_key(|&(x, _)| x); | ||
assert!(v.windows(2).all(|w| w[0] <= w[1])); | ||
} | ||
} | ||
} | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to be able to use
SmallVec
here and save an allocation when sorting short slices.Just a suggestion, though. Maybe leave that for a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea; I might leave that for a future PR though, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than pulling in a full crate (which is mostly about encapsulating stuff in a single type that can be moved as a unit), you can reproduce that functionality with a couple of stack variables. Something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just a caveat, that's the gist of it and the array code needs to take more precautions to be safe.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluss Oh, do you mean panic-safe? Yes, I forgot about that, the array should be inside a
ManuallyDrop
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use
ptr::write
or the equivalent with a ManuallyDrop around each element, that's the two things I could spot.