Add max and min to Ord #42496

Razaekel · 2017-06-07T03:03:24Z

Pursuant to issue #25663, this PR adds max and min methods with default implementations to std::cmp::Ord. It also modifies std::cmp::max|min to internally alias to Ord::max|min, so that any overrides of the default implementations are automatically used by std::cmp::max|min.

Closes #25663

Pursuant to issue #25663, this commit adds the max and min functions to the Ord trait, enabling items that implement Ord to use UFCS (ex. 1.max(2)) instead of the longer std::cmp::max(1,2) format.

rust-highfive · 2017-06-07T03:03:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2017-06-07T04:06:56Z

src/libcore/cmp.rs

+    /// ```
+    #[unstable(feature = "ord_max_min", issue = "25663")]
+    fn max(self, other: Self) -> Self
+    where Self: Sized {


Not sure if this is the best place to discuss this, but I think both of these could take (&self, other: &Self) and allow us to relax the Self: Sized bound -- getting an ordering doesn't require by-value access.

I chose to use by-value access because that's the same as what the std::cmp::max|min code below this does, and this PR is essentially moving those fns into Ord.

Plus, this isn't getting an Ordering the way cmp above is, but moving/copying both values in, comparing them, and then returning one. I think if I took (&self, other: &Self), I'd have to return a &Self, which would violate the API defined by std::cmp::max|min.

I think it makes sense to keep them for now, but wanted to bring it up since maybe others will want to change it "while we're at it."

I don't have an issue with it either way, I was just keeping things similar to what already existed since that results in less surprises.

I agree that T is a more helpful return type than &T.

Also, #25663 (comment) argues for consistency with the inherent impl on fN:

I often write performance-sensitive graphics code, which I prototype on floats for ease of writing, and then where appropriate change to use integers for speed/memory efficiency. 90% of calculations just work, and max is one of the warts that doesn't.

Shame that a.min(&b) fails to compile instead of auto-deref'ing to (&a).min(&b), as it'd be an elegant way to use the impl<T:Ord> Ord for &T where borrow comparison is desirable...

carols10cents · 2017-06-12T12:27:13Z

Aturon's on vacation this week, sorry this fell through the cracks last week! Let's try...

r? @BurntSushi

BurntSushi · 2017-06-13T10:57:20Z

LGTM! Thank you! @bors r+

bors · 2017-06-13T10:57:21Z

📌 Commit a32ffc6 has been approved by BurntSushi

bors · 2017-06-13T16:44:50Z

⌛ Testing commit a32ffc6 with merge 378887b...

bors · 2017-06-13T20:54:05Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-06-13T20:56:51Z

@bors retry

os x timeouts

…=BurntSushi Add max and min to Ord Pursuant to issue rust-lang#25663, this PR adds max and min methods with default implementations to std::cmp::Ord. It also modifies std::cmp::max|min to internally alias to Ord::max|min, so that any overrides of the default implementations are automatically used by std::cmp::max|min. Closes rust-lang#25663

Rollup of 6 pull requests - Successful merges: #42408, #42428, #42496, #42597, #42636, #42638 - Failed merges: #42612

bors · 2017-06-14T02:56:28Z

☔ The latest upstream changes (presumably #42644) made this pull request unmergeable. Please resolve the merge conflicts.

The current Rust nightly introduced a breaking change [1] that is causing the build to fail for the nightly channel. The affected crate, petgraph, has been updated [2] so it now compiles on nightly again, but unfortunately I cannot use the latest version of that crate, because it is a transitive dependency, and the crate which depends on it depends on a version that Cargo considers semver-incompatible with the fixed crate. The crate which depends on the crate broken by Rust nightly is lalrpop, and a new version of it has been released that depends on a newer version of petgraph which *is* semver-compatible with the fixed release. However, upgrading to a newer version of lalrpop is also not possible, because it requires a newer version of Rust. (Even after I contributed a fix for that, and despite the changelog claiming to support Rust 1.13 now, it is still broken on 1.13 because of the lalrpop-snap crate.) So now I am in a situation where I either have to drop support for Rust 1.13 and upgrade dependencies, or drop support for nightly Rust. The least invasive change to my code at this point, is to drop support for the nightly channel. Unfortunately that is only a temporary solution, because the breakage is going to make it into beta and then stable. But I am working on a hand-written parser anyway which will replace lalrpop, so that dependency, and the dependency on petgraph, can hopefully be dropped soon. That would maintain compatibility with Rust 1.13, and restore compatibility with nightly at the same time. A third option would be to ask the petgraph maintainers to release a patch release of the older version that lalrpop depends on, which was broken by Rust nightly. But for now the easiest thing for me to do to get the build green, is to drop support for nightly Rust. [1]: rust-lang/rust#42496 [2]: petgraph/petgraph#153

arielb1 · 2017-08-05T14:53:33Z

This had causes quite a few regressions in code. Do we still want this in this form?

BurntSushi · 2017-08-09T00:52:53Z

Is there a tracking issue for these regressions? There do seem to be a fair number of them. cc @rust-lang/libs

aturon · 2017-08-09T15:20:39Z

@BurntSushi I don't know of any tracking issue for regressions. Looks like there are 7 linking to this issue, with only 2 still open. I consider this an acceptable count, given that this was permissible breakage.

alexcrichton · 2017-08-09T17:45:32Z

This did I think cause more regressions than we thought was going to happen. In that sense I wouldn't be opposed if it was proposed to be reverted, but I also don't think I'd advocate for reverting this.

rust-lang/rust#42496 means that 0.4.4 no longer compiles with the most recently released rust toolchain. This was fixed in petgraph/petgraph#154 which is in 0.4.5.

Lower the priority of unstable methods when picking a candidate. Previously, when searching for the impl of a method, we do not consider the stability of the impl. This leads to lots of insta-inference-regressions due to method ambiguity when a popular name is chosen. This has happened multiple times in Rust's history e.g. * `f64::from_bits` #40470 * `Ord::{min, max}` #42496 * `Ord::clamp` #44095 (eventually got reverted due to these breakages) * `Iterator::flatten` #48115 (recently added) This PR changes the probing order so that unstable items are considered last. If a stable item is found, the unstable items will not be considered (but a future-incompatible warning will still be emitted), thus allowing stable code continue to function without using qualified names. Once the unstable feature is stabilized, the ambiguity error will still be emitted, but the user can also use newly stable std methods, while the current situation is that downstream user is forced to update the code without any immediate benefit. (I hope that we could bring back `Ord::clamp` if this PR is merged.)

Razaekel added 2 commits June 6, 2017 22:42

Add max and min default fns to Ord trait

2cadc32

Pursuant to issue #25663, this commit adds the max and min functions to the Ord trait, enabling items that implement Ord to use UFCS (ex. 1.max(2)) instead of the longer std::cmp::max(1,2) format.

Alias std::cmp::max/min to Ord::max/min

a32ffc6

rust-highfive assigned aturon Jun 7, 2017

Mark-Simulacrum reviewed Jun 7, 2017

View reviewed changes

Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 7, 2017

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2017

rust-highfive assigned BurntSushi and unassigned aturon Jun 12, 2017

arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2017

frewsxcv mentioned this pull request Jun 13, 2017

Rollup of 6 pull requests #42644

Merged

bors added a commit that referenced this pull request Jun 14, 2017

Auto merge of #42644 - frewsxcv:rollup, r=frewsxcv

e40ef96

Rollup of 6 pull requests - Successful merges: #42408, #42428, #42496, #42597, #42636, #42638 - Failed merges: #42612

bors merged commit a32ffc6 into rust-lang:master Jun 14, 2017

dtolnay mentioned this pull request Jun 15, 2017

Multiple max found petgraph/petgraph#153

Closed

This was referenced Aug 5, 2017

glm-0.2.3 beta regression #43667

Closed

as_num-0.2.2 beta regression #43657

Closed

direct-0.1.0 beta regression #43665

Closed

tree-0.4.1 beta regression #43682

Closed

statrs 0.6.0 and 0.7.0 beta regression #43680

Closed

scottmcm mentioned this pull request Aug 9, 2017

Add clamp RFC rust-lang/rfcs#1961

Merged

This was referenced Aug 22, 2017

Conflicting methods min and max on Set and Map apasel422/tree#23

Closed

Fix breakage with Order.min and Order.max on nightly dche/glm-rs#21

Merged

Razaekel deleted the feature/integer_max-min branch August 29, 2017 17:49

illicitonion mentioned this pull request Sep 1, 2017

Update petgraph dep to 0.4.5 pantsbuild/pants#4837

Closed

scottmcm mentioned this pull request Sep 11, 2017

Tracking issue for clamp RFC #44095

Closed

3 tasks

cristicbz mentioned this pull request Jan 11, 2018

Use of unstable library feature 'ord_max_min' cristicbz/rust-doom#121

Closed

kennytm mentioned this pull request Feb 26, 2018

Lower the priority of unstable methods when picking a candidate. #48552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add max and min to Ord #42496

Add max and min to Ord #42496

Razaekel commented Jun 7, 2017 •

edited

Loading

rust-highfive commented Jun 7, 2017

Mark-Simulacrum Jun 7, 2017

Razaekel Jun 7, 2017 •

edited

Loading

Mark-Simulacrum Jun 7, 2017

Razaekel Jun 7, 2017 •

edited

Loading

scottmcm Jun 7, 2017

carols10cents commented Jun 12, 2017

BurntSushi commented Jun 13, 2017

bors commented Jun 13, 2017

bors commented Jun 13, 2017

bors commented Jun 13, 2017

Mark-Simulacrum commented Jun 13, 2017

bors commented Jun 14, 2017

arielb1 commented Aug 5, 2017

BurntSushi commented Aug 9, 2017

aturon commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

Add max and min to Ord #42496

Add max and min to Ord #42496

Conversation

Razaekel commented Jun 7, 2017 • edited Loading

rust-highfive commented Jun 7, 2017

Mark-Simulacrum Jun 7, 2017

Choose a reason for hiding this comment

Razaekel Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Mark-Simulacrum Jun 7, 2017

Choose a reason for hiding this comment

Razaekel Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

scottmcm Jun 7, 2017

Choose a reason for hiding this comment

carols10cents commented Jun 12, 2017

BurntSushi commented Jun 13, 2017

bors commented Jun 13, 2017

bors commented Jun 13, 2017

bors commented Jun 13, 2017

Mark-Simulacrum commented Jun 13, 2017

bors commented Jun 14, 2017

arielb1 commented Aug 5, 2017

BurntSushi commented Aug 9, 2017

aturon commented Aug 9, 2017

alexcrichton commented Aug 9, 2017

Razaekel commented Jun 7, 2017 •

edited

Loading

Razaekel Jun 7, 2017 •

edited

Loading

Razaekel Jun 7, 2017 •

edited

Loading