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.

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

std::cmp::max vs std::num::Float vs NaN #852

Closed
steveklabnik opened this Issue Feb 15, 2015 · 4 comments

Comments

Projects
None yet
5 participants
@steveklabnik
Copy link
Member

steveklabnik commented Feb 15, 2015

Issue by pornel
Saturday Jan 31, 2015 at 18:28 GMT

For earlier discussion, see rust-lang/rust#21816

This issue was labelled with: A-libs in the Rust repository


It's werid that std::cmp::max can't compare f64 numbers. I know that strictly speaking IEEE floats don't have total order this function expects, but still it's surprising (and partial_max is awkward to use).

And there's std::num::Float::max which works with f64 just fine (the docs don't even say how NaN is handled).

It bugs me that the two versions of max are not consistent in their strictness, and that the first-and-most-obvious max function in the stdlib "doesn't work" with a basic type in the language.

My suggestion:

  • Rename the max version that only allows Ord to something else, like total_max or strict_max.
  • Implement std::cmp::max for floating point numbers, so that a.max(b) is consistent with max(a,b).
@barosl

This comment has been minimized.

Copy link
Contributor

barosl commented Jun 17, 2015

I stumbled upon this issue after reading a comment from HN (which was posted by the issue author, by the way). While I'm more inclined to not have max and min impls for the primitive types f64 and f32, they are now stable and will not change in the future.

Regarding the documentation concerns, rust-lang/rust#25951 made it clear how NaN is handled in those functions. So at least their behaviors are well-defined now.

Also reading rust-lang/rust#25663, @alexcrichton said that cmp::max and cmp::min (and their PartialOrd counterparts) should be more recommended methods for numeric comparisons. Does it mean that we should also document the existence of those methods in {f64, f32}::{max, min} and encourage their uses? cc @bluss @aturon

@xaviershay

This comment has been minimized.

Copy link

xaviershay commented Sep 26, 2015

I was confused by this. It's not immediately obvious why max shouldn't work for floats - in practical usage you don't come across NaN all that often.

@xaviershay

This comment has been minimized.

Copy link

xaviershay commented Sep 26, 2015

std::num::Float::max not a thing anymore apparently, but 0.0.max(1.0) works.

@petrochenkov petrochenkov added T-libs and removed A-libs labels Jan 28, 2018

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 26, 2018

I'm going to deem this resolved with the addition of Ord::min/max, as now all code can use the min method consistently -- the inherent for floats, the trait method for Ord.

Feel free to comment or reopen if you disagree.

@scottmcm scottmcm closed this Feb 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.