Stable sort primitive #5809
Stable sort primitive #5809
Conversation
|
r? @phansch (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Sorry for the silence and thanks for your PR - I'll get back to reviewing by tomorrow =) |
|
@phansch Is the review still coming? |
|
ping from triage @phansch. This is still waiting on your review. If you are currently too busy, you can |
|
Sorry again for the silence, got some IRL stuff to deal with that drains my energy, so I'm handing this off to someone else 💙 r? @Manishearth |
tests/ui/unnecessary_sort_by.rs
Outdated
| @@ -9,7 +9,6 @@ fn unnecessary_sort_by() { | |||
|
|
|||
| let mut vec: Vec<isize> = vec![3, 6, 1, 2, 5]; | |||
| // Forward examples | |||
| vec.sort_by(|a, b| a.cmp(b)); | |||
There was a problem hiding this comment.
We should keep this test until we implement unstable sort lint for sort_by (SortingKind::ByCmp)
| // positive examples | ||
| let mut vec = vec![1, 3, 2]; | ||
| vec.sort_unstable(); | ||
| let mut vec = vec![false, false, true]; |
There was a problem hiding this comment.
Should we add a test for chars as well?
There was a problem hiding this comment.
Also test cases for types that deref to a slice of primitives.
There was a problem hiding this comment.
I believe Vec derefs to a slice, so I think the tests already have that last bit covered, unless I don't understand what you're getting at.
There was a problem hiding this comment.
No, you are correct. My bad.
There was a problem hiding this comment.
@JarredAllen Hey should we add test cases for list of tuples such as: vec![(2, 1), (1, 3), (3, 2)]
There was a problem hiding this comment.
@krishna-veerareddy I added test cases for a list of tuples and also for one of arrays.
|
|
||
| /// Returns true iff the given type is a primitive which we should lint | ||
| /// on. | ||
| fn is_primitive_type(ty: Ty<'_>) -> bool { |
There was a problem hiding this comment.
Can we extend this to include a list of static strings such as ["ab", "bc", "ab", "cd"]. This would qualify for an unstable sort, right?
There was a problem hiding this comment.
That should be a Ref of a Slice of a Str
Can we rename this function is_recursively_primitive_type and stick it in utils somewhere?
JarredAllen
force-pushed
the
stable_sort_primitive
branch
from
408e26c
to
3d0628d
Compare
|
@krishna-veerareddy I'll let you review this PR since you've already started, let me know when you think it's ready! |
|
@Manishearth Hey this looks good to me. Can you please take a look when you have some time. Thanks! |
| /// The three "kinds" of sorts | ||
| enum SortingKind { | ||
| Vanilla, | ||
| /* The other kinds of lint are currently commented out because they |
There was a problem hiding this comment.
I would avoid using multiline comments and instead just use lots of //, it's pretty standard style in Rust or at least in this codebase.
There was a problem hiding this comment.
For some reason, cargo dev fmt seems to insist on changing those comments to be multiline ones.
|
|
||
| /// Returns true iff the given type is a primitive which we should lint | ||
| /// on. | ||
| fn is_primitive_type(ty: Ty<'_>) -> bool { |
There was a problem hiding this comment.
That should be a Ref of a Slice of a Str
Can we rename this function is_recursively_primitive_type and stick it in utils somewhere?
|
|
||
| /// Returns true iff the given expression is a slice of primitives or if | ||
| /// it is of some type that implements deref to such a slice | ||
| fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { |
JarredAllen
force-pushed
the
stable_sort_primitive
branch
from
1892a57
to
25abd7a
Compare
|
@bors r+ |
|
📌 Commit 25abd7a has been approved by |
Stable sort primitive changelog: Implements #5762
|
💔 Test failed - checks-action_dev_test |
|
Needs a |
|
@flip1995 The only changed caused by running |
|
Yes please. I'm surprised it does that, but whatever 😄 |
|
@bors r+ |
|
📌 Commit 542740c has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: Implements #5762