Skip to content

Tracking issue for Ord::{min, max} #25663

Closed
@kornelski

Description

@kornelski
Contributor

It's seems inconsistent that floating-point types have a min method, but integer types don't.

And there's a standalone min() function that works with integer types, but it's not defined for floating-point types.

I don't have a strong preference for either style, but I'd prefer that at least one way of computing min/max was available for both floating point and integer types alike.

Activity

added
T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.
on May 21, 2015
Aatch

Aatch commented on May 21, 2015

@Aatch
Contributor

@aturon thoughts? Seems like adding an inherent method to integer types is the easiest way forward here.

Note that there are partial_min and partial_max functions for types that only implement PartialOrd.

aturon

aturon commented on May 21, 2015

@aturon
Member

+1 for an inherent method. I think this is just an oversight.

alexcrichton

alexcrichton commented on May 26, 2015

@alexcrichton
Member

I believe @bluss commented to this effect already, but the idea here is that there is "one true min method" at cmp::min, but the floating point types use a slightly different algorithm (e.g. different guarantees about order and whatnot), hence they have their own methods.

I wouldn't be totally opposed to adding an inherent method, but we'd want to clearly document that it's just an alias to cmp::min

nagisa

nagisa commented on Jun 17, 2015

@nagisa
Member

-1 for an inherent (or any other, really) method. Float’s min and max are just bindings to libm f{min.max}{f,}, while cmp::partial_{min,max} have their own, sane if you will, semantics. Adding inherent methods for integral types will make many-ways-to-do-the-same problem worse.

frewsxcv

frewsxcv commented on Dec 14, 2016

@frewsxcv
Member

Ignoring the "many-ways-to-do-the-same problem" for a second, couldn't min and max just be generic 'provided methods' on the Ord trait?

kornelski

kornelski commented on Dec 14, 2016

@kornelski
ContributorAuthor

For me this is a practical problem, because changing type needlessly changes syntax.

let a = 1.0;
let b = 2.0;
let c = a.max(b); // c = std::cmp::max(a,b)

I can't change a = 1; b = 2 without rewriting the third line.

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.

Mark-Simulacrum

Mark-Simulacrum commented on May 18, 2017

@Mark-Simulacrum
Member

@rust-lang/libs: Looks like this needs some thought; I've nominated. Possibly interesting for libz blitz discussion, too, since it relates to number types in wider library set; for example serde_json::Number doesn't implement min/max inherently, but should it?

I think that the things that have been brought up are:

aturon

aturon commented on May 23, 2017

@aturon
Member

We discussed this in triage today, and the libs team is in favor of adding these methods to Ord

Mark-Simulacrum

Mark-Simulacrum commented on May 23, 2017

@Mark-Simulacrum
Member

[De-nominating]

10 remaining items

rfcbot

rfcbot commented on Aug 29, 2017

@rfcbot
Collaborator

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

added
proposed-final-comment-periodProposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
on Aug 29, 2017
rfcbot

rfcbot commented on Sep 12, 2017

@rfcbot
Collaborator

🔔 This is now entering its final comment period, as per the review above. 🔔

added
final-comment-periodIn the final comment period and will be merged soon unless new substantive objections are raised.
and removed
proposed-final-comment-periodProposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
on Sep 12, 2017
added a commit that references this issue on Sep 15, 2017

Rollup merge of rust-lang#44593 - budziq:stabilize_ord_max_min, r=ale…

5e9b68f
added a commit that references this issue on Sep 16, 2017

Rollup merge of rust-lang#44593 - budziq:stabilize_ord_max_min, r=ale…

adc0961
added a commit that references this issue on Sep 16, 2017

Rollup merge of rust-lang#44593 - budziq:stabilize_ord_max_min, r=ale…

1ae6ab4
added a commit that references this issue on Sep 25, 2017
df8fec1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    B-unstableBlocker: Implemented in the nightly compiler and unstable.C-tracking-issueCategory: An issue tracking the progress of sth. like the implementation of an RFCT-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.final-comment-periodIn the final comment period and will be merged soon unless new substantive objections are raised.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @alexcrichton@kornelski@Aatch@frewsxcv@nagisa

        Issue actions

          Tracking issue for Ord::{min, max} · Issue #25663 · rust-lang/rust