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.

Sign up for GitHub

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

Jump to bottom

Added core::cmp::Reverse for sort_by_key reverse sorting #40720

Merged
merged 5 commits into from
Mar 29, 2017
Merged

Added core::cmp::Reverse for sort_by_key reverse sorting #40720

merged 5 commits into from
Mar 29, 2017

Conversation

mitsuhiko
Copy link
Contributor

I'm not sure if this is the best way to go about proposing this feature but it's pretty useful. It allows you to use sort_by_key and return tuples where a single item is then reversed to how it normally sorts.

I quite miss something like this in Rust currently though I'm not sure if this is the best way to implement it.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler
Copy link
Member

This seems like a nice ergonomic win to me. Thoughts @rust-lang/libs?

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 22, 2017
@ranma42
Copy link
Contributor

ranma42 commented Mar 22, 2017

It would be possible to relax the constraints a bit, so that T is allowed to only implement partial orderings, like this.

@nagisa
Copy link
Member

nagisa commented Mar 22, 2017

Stability attributes should be fixed.

@malbarbo
Copy link
Contributor

How about a free function cmp::reverse? Reverse seems to generic...

@mitsuhiko
Copy link
Contributor Author

@malbarbo what would the function do? Create a "Reverse" object?

@malbarbo
Copy link
Contributor

@mitsuhiko Yes. This is similar to free functions in iter module.

@mitsuhiko
Copy link
Contributor Author

I was going with std::num::Wrapping in mind which is also just a wrapper / newtype over something. Feels wasteful to add a function that does nothing other than making an enum which needs to be public anyways.

@mitsuhiko
Copy link
Contributor Author

Alternative name proposals:

  • std::cmp::Rev
  • std::cmp::Invert
  • std::cmp::Inv

@alexcrichton
Copy link
Member

Seems like a nice addition to me!

@alexcrichton
Copy link
Member

I personally like Reverse but would be ok with Invert as well.

@scottmcm
Copy link
Member

Good to see it handling PartialOrd as well now.

I don't like Invert, since that sounds like 1/x. I'm not a fan of just Reverse either, since v.reverse() and Reverse(v) would both compile but do very different things.

Maybe ReverseOrder? This doesn't feel like something that needs a particularly short name.

Or maybe something involving Descending? v.sort_by_key(|k| Descending(k)) reads decently...

aturon
aturon reviewed Mar 28, 2017
View reviewed changes
src/libcore/cmp.rs Outdated
/// assert_eq!(v, vec![3, 2, 1, 6, 5, 4]);
/// ```
#[derive(PartialEq, Eq, Debug)]
#[stable(feature = "rust1", since = "1.18.0")]
Copy link
Member

Choose a reason for hiding this comment

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

This will need to start out unstable.

@mitsuhiko
Copy link
Contributor Author

In my own project I called it RevKey but I found that confusing. I do like the idea of Descending which is what some SQL frameworks do. For instance in Python's SQLAlchemy column sorts upwards and column.desc() sorts backwards.

@aturon
Copy link
Member

aturon commented Mar 28, 2017

We discussed this PR in the libs triage meeting today, and everyone was in favor of the addition. We're also fine with both the name and API. (It's a slight shift from other APIs in std, in that it exposes a constructor for a tuple struct directly, but that is where idioms have headed in general).

Other than changing the annotations to unstable, I think this is good to land. r=me after that change.

@aturon
Copy link
Member

aturon commented Mar 28, 2017

@mitsuhiko Ah sorry, to clarify the unstable attribute will need to point to a fresh "tracking issue" like #40494 (make sure to tag B-unstable). You can go ahead and create that.

@mitsuhiko mitsuhiko mentioned this pull request Mar 29, 2017
Closed
@mitsuhiko
Copy link
Contributor Author

@aturon i created the issue but lack the rights to flag it appropriately. So it's untagged for now.

@BurntSushi
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 29, 2017

📌 Commit 5d36953 has been approved by BurntSushi

@BurntSushi
Copy link
Member

Thanks @mitsuhiko! :-)

@mitsuhiko
Copy link
Contributor Author

Wohoo \o/ Thanks :)

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 29, 2017
@frewsxcv
Rollup merge of rust-lang#40720 - mitsuhiko:feature/rev-key, r=BurntS…
978c906 
…ushi

Added core::cmp::Reverse for sort_by_key reverse sorting

I'm not sure if this is the best way to go about proposing this feature but it's pretty useful. It allows you to use `sort_by_key` and return tuples where a single item is then reversed to how it normally sorts.

I quite miss something like this in Rust currently though I'm not sure if this is the best way to implement it.
@frewsxcv frewsxcv mentioned this pull request Mar 29, 2017
Merged
bors added a commit that referenced this pull request Mar 29, 2017
@bors
Auto merge of #40899 - frewsxcv:rollup, r=frewsxcv
e1cec5d 
Rollup of 5 pull requests

- Successful merges: #40720, #40786, #40841, #40866, #40897
- Failed merges:
@bors bors merged commit 5d36953 into rust-lang:master Mar 29, 2017
@bluss bluss mentioned this pull request Mar 30, 2017
Merged
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 31, 2017
@frewsxcv
Rollup merge of rust-lang#40929 - bluss:full-reverse, r=alexcrichton
d36b24d 
Implement all PartialOrd methods for Reverse

When making a forwarding wrapper we must in general forward all methods,
so that we use the type's own `lt` for example instead of the default.

Example important case: f32's partial_cmp does several operations but
its lt is a primitive.

Follow up on rust-lang#40720
@mitsuhiko mitsuhiko deleted the feature/rev-key branch April 4, 2017 21:17
@malbarbo malbarbo mentioned this pull request May 24, 2017
Closed
rkjnsn
rkjnsn reviewed Jun 2, 2017
View reviewed changes
src/libcore/cmp.rs
/// v.sort_by_key(|&num| (num > 3, Reverse(num)));
/// assert_eq!(v, vec![3, 2, 1, 6, 5, 4]);
/// ```
#[derive(PartialEq, Eq, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we derive PartialEq and Eq here, when we are specifying custom implementations below?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no custom implementation for PartialEq, only for Ord and PartialOrd.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, of course. I don't know where my brain was. Sorry about that.

@dtolnay dtolnay mentioned this pull request Nov 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkjnsn rkjnsn rkjnsn left review comments

@ranma42 ranma42 ranma42 left review comments

@aturon aturon aturon left review comments

Assignees

@sfackler sfackler

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.