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

Add a total ordering method for floating-point #53938

Closed
wants to merge 1 commit into from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Sep 4, 2018

AddsfN::total_cmp following the IEEE totalOrder rules, inspired by a discussion with @rkruppe.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2018
@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 4, 2018
Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

Thank you so much for tackling this! Unfortunately I have lots of nits wrt the docs, in particular the bullet points defining the order. I have a different approach in mind for that part, I'll write it up and post it here for comparison rather than ask you to reword each bullet point individually.

///
/// Because of negative zero and NaNs, the ordinary comparison operators
/// for `f32` do not represent a total order. This method, however,
/// defines an ordering between all distinct bit patterns:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'd want to mention this, since Rust pretty much ignores non-canonical floats anyway, but I'll mention it for completeness: totalOrder only distinguishes all bit patterns of the exchange format (binary32), the arithmetic format may have multiple distinct encodings of the same floating point datum, which would then all be considered equal.

Copy link

Choose a reason for hiding this comment

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

What do you mean by "non-canonical floats"? Only +0 and -0 are distinct encodings of sorta-equal numbers, and they are explicitly distinct in this ordering. There are no other cases where two distinct bit patterns represent equal IEEE754 floats.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true of the standard/basic formats (in radix 2 at least), but an implementation may perform operations on a different format (the "arithmetic format") and provide conversions between that format and the interchange formats, and then the arithmetic format may have multiple encodings for what would be one datum with only one encoding in the basic format. Some examples can be found in https://llvm.org/docs/LangRef.html#llvm-canonicalize-intrinsic

/// for `f32` do not represent a total order. This method, however,
/// defines an ordering between all distinct bit patterns:
///
/// - For normal numbers, the ordering matches the comparison operators.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Normal" has a specific meaning for floats which you probably don't want here (it excludes subnormals and infinities, which totalOrder also handles exactly like the ordinary comparisons). It also isn't great for lay people since it's not obvious that "normal" excludes zeros.

/// defines an ordering between all distinct bit patterns:
///
/// - For normal numbers, the ordering matches the comparison operators.
/// - Zeros are between the positive and negative normal numbers, with
Copy link
Contributor

@hanna-kruppe hanna-kruppe Sep 4, 2018

Choose a reason for hiding this comment

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

It took me a moment to parse this sentence as {negative non-zero} < -0.0 < +0.0 < {positive non-zero} (see above comment re: "normal"). It's also not great that this makes the ordering between zeros and non-zero finite numbers sound like a deviation from the conventional comparisons when really only the distinction between -0 and +0 is new.

/// - Zeros are between the positive and negative normal numbers, with
/// the positive zero greater than the negative zero.
/// - Positive infinity is greater than all normal numbers; negative
/// infinity is less than all normal numbers.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in line with the usual comparison operators, and again "normal" is the wrong (overly narrow) term here.

/// - Positive infinity is greater than all normal numbers; negative
/// infinity is less than all normal numbers.
/// - Positive NaNs are greater than all other values, ordered amongst
/// themselves by their payloads; Negative NaNs are less than all other
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that NaNs are ordered by signaling bit first, payload second (and the signaling bit is not part of the payload in official terms). I believe with the 2008 definition of the signaling bit (1 for quiet, 0 for signaling) this turns out the same as ordering by the significand bits, but since we did in the past take at least a little care about every existing Mips chip having this bit the other way around, maybe we should be a bit more precise (or maybe even use a different implementation on Mips to respect its interpretation of the signaling bit).

cc @est31

Copy link
Member

Choose a reason for hiding this comment

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

maybe even use a different implementation on Mips to respect its interpretation of the signaling bit

It seems that the totalOrder operation got introduced by the 2008 edition of IEEE 754, i.e. it wasn't present in the 1985 edition. The totalOrder operation is specified in terms of the payload and the signaling bit, and not the signifigand bits. If we implement the operation on older MIPS, which is only compliant with the 1985 edition, we are basically "backporting" the definition in the 2008 standard to the 1985 standard. If we don't adjust the parts about the signaling bit, I think this goes really wrong: I think that the spec authors have carefully made the totalOrder rules so that they can be implemented by a simple operation on the binary representation. The totalOrder spec was made with the 2008 definition in mind, not with the interpretation of 1985 that MIPS chose. We should follow that spirit on MIPS as well and take the implementation that is a simple operation not a "if this is NAN then flip this bit and then do $op" one.

I don't think that @rkruppe is considering this too seriously, but just wanted to say this.

And yeah, we should document this choice about the MIPS+NaN behaviour and maybe say that it might be changed it in the future, to keep options open.

/// a.sort_by(|&x, &y| f32::total_cmp(x, y));
/// assert_eq!(
/// format!("{:?}", a),
/// "[NaN, -inf, -1.0, -0.0, 0.0, 1.0, inf, NaN]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of unfortunate that we don't print -NaN with the -.

Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone following along; this was discussed further in #54235.

fn total_order_key(self) -> i32 {
let x = self.to_bits() as i32;
// Flip the bottom 31 bits if the high bit is set
x ^ ((x >> 31) as u32 >> 1) as i32
Copy link
Contributor

Choose a reason for hiding this comment

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

This really, really needs either an in-depth explanation justifying its correctness, or a reference to an external write-up of such an explanation. It sounds really plausible but I'm also kind of unsure about various aspects, and I've already spent more time thinking about this order and about the binary exchange format than any human being should.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a proof for this, but it's equivalent to the one in rust-lang/rfcs#1249 (comment). I suspect the predicate was chosen specifically to allow such a simple implementation, because of things like "the same floating-point datum" being ordered by exponent first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the exact same question for @jpetkau then 😉 I agree with your suspicion (tho note that "same floating point datum" with different exponents only happens in decimal floating point) but I'd rather have it confirmed.

Copy link

Choose a reason for hiding this comment

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

How about something like:

"With their bits interpreted as signed integers, positive IEEE754 floats are ordered as:

+0 < (positive finite floats in order) < +inf < +NANs.

With the reverse order for floats with the sign bit set.

So if we reverse the order of the negative cases, by xoring the sign bit with the other bits, we get the order:

-NANs < -inf < (negative finite floats in order) < -0 < +0 < (positive finite floats in order) < +inf < +NANs.

IEEE754.2008 calls this the 'totalOrder' predicate.

(It also specifies an interpretation of the signaling bit which implies -QNANs < -SNANs < +SNANs < +QNANs, but if we happen to be on an older MIPS architecture with the opposite interpretation of the signaling bit, this won't be true.)

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds mostly convincing! The main thing that isn't obvious to me is why negating the non-sign bits reverses the order but maybe that's just someone you have to convince yourself of by spot checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that this reinterprets the whole 32- or 64-bit unit, but it's possible that an architecture uses different endianness for floats and ints, then the bytes will appear reversed when stored to memory as a float and loaded back as an integer. However, I don't think we currently support such architectures, and if we wanted to, we'd have to update more code than just this method (in particular to_bits, which would automatically make this code work correctly too).

Copy link

@jpetkau jpetkau Oct 25, 2018

Choose a reason for hiding this comment

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

I dispute that it's possible that an architecture uses different endianness for floats and ints. That would be deeply insane and break a lot more than this code.

[edit] of course it's technically possible, just like an architecture could use different endianess for signed and unsigned ints if it really wanted to be perverse.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean you dispute that it's possible? There's real physical processors (old ARMs) doing so. We don't support them, and I would be open to concluding we don't ever want to support them (just like e.g. architectures with non-octet memory granularity – also breaks tons of code, but definitely a real thing!), but they exist.

Copy link

Choose a reason for hiding this comment

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

Sorry, I was just being flippant there. Such architectures do exist. But they would break far more than just this code; they'd break everything that relied on the bit pattern of floats, which is, well, everything. Every serialization library, formatter, parser, implementation of math libraries, you name it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Endianness doesn't matter, because an f32 and the i32 will still have the same endianness as each other.

Thanks for the answer, I just wasn't sure if this had to be the case, but it seems that for Rust this will need to be the case.

///
/// # Examples
///
/// Normal numbers:
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the section demonstrating how this function agrees with the PartialOrd impl, I'm missing:

  • Comparisons between non-zero finite numbers and zeros
  • Comparisons between infinities and finite numbers

/// assert_eq!(f32::total_cmp(-2.0, -1.0), Less);
/// ```
///
/// Zeros, infinities, and NaNs:
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above infinities are not any different from the PartialOrd impl so it's misleading to group them in with zeros and NaNs imo.

///
/// assert_eq!(f32::partial_cmp(&std::f32::NAN, &std::f32::NAN), None);
/// assert_eq!(f32::total_cmp(-std::f32::NAN, std::f32::NAN), Less);
/// assert_eq!(f32::total_cmp(std::f32::NAN, std::f32::NAN), Equal);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 We should probably have examples demonstrating the ordering by signaling bit and payload, constructing some non-default NaNs via from_bits.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 4, 2018

How about we document the total order by first summarizing how it almost entirely agrees with PartialOrd and how it orders NaNs, then explicitly listing all relevant groups of floating point data in the order they're put in. Something like this:


This function mostly agrees with PartialOrd and the usual comparison operators in how it orders floating point numbers, but it additionally distinguishes negative from positive zero (it considers -0 as less than +0, whereas == considers them equal) and it orders Not-a-Number values relative to the numbers and distinguishes NaNs with different bit patterns.

NaNs with positive sign are ordered greater than all other floating-point values including positive infinity, while NaNs with negative sign are ordered the least, below negative infinity. Two different NaNs with the same sign are ordered first by whether the are signaling (signaling is less than quiet if the sign is positive, reverse if negative) and second by their payload interpreted as integer (reversed order if the sign is negative).

This means all different (canonical) floating point bit patterns are placed in a linear order, given below in ascending order:

  • Quiet Not-a-Number with negative sign (ordered by payload, descending)
  • Signaling Not-a-Number with negative sign (ordered by payload, descending)
  • Negative infinity
  • Negative finite non-zero numbers (ordered in the usual way)
  • Negative zero
  • Positive zero
  • Positive finite non-zero numbers (ordered in the usual way)
  • Positive infinity
  • Signaling Not-a-Number with positive sign (ordered by payload, ascending)
  • Quiet Not-a-Number with positive sign (ordered by payload, ascending)

@hanna-kruppe
Copy link
Contributor

cc rust-lang/rfcs#1249 @glaebhoerl

/// ```
#[unstable(feature = "float_total_cmp", issue = "88888888")]
#[inline]
pub fn total_cmp(self, other: f32) -> cmp::Ordering {
Copy link
Member

Choose a reason for hiding this comment

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

Could this function take references like Ord::cmp does? It would allow writing things like a.sort_by(f32::total_cmp).

Copy link
Member Author

Choose a reason for hiding this comment

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

I contemplated that, but decided that would be inconsistent with all the other things that would take references if f32 wasn't copy, but takes owned because it is copy. I note that there's currently not a single &self or &f32 in the inherent impl methods today.

Hopefully the copy type ergonomics stuff and better fn type coercions will make .sort_by(f32::total_cmp) just work sometime in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering about this too. If one just can't .sort_by(f32::total_cmp) people will definitely reach to unsafe code for going from &[f32]s to &[Total<f32>] and vice-versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the other solution to that would be to just make it safe...

@hanna-kruppe
Copy link
Contributor

The doc comment should mention somewhere prominent that despite its lengthy definition, this is implemented really efficiently with bit twiddling and integer comparisons, to encourage people to use it. I was thinking of combining this with a one sentence summary of the intent of the method (a total order for floats that is sensible but also orders NaNs). Currently we just have a reference to the standard's totalOrder, which won't mean anything to most people, and then dive straight into the details.

@Mark-Simulacrum
Copy link
Member

r? @rkruppe

@ghost
Copy link

ghost commented Sep 11, 2018

Is there a reason why we're not relying on LLVM to compare floats rather than fiddling with bitwise operations? Is it perhaps inconsistent with the exact ordering we'd like to guarantee?

I'm curious because LLVM can probably emit more efficient instructions for comparing floats than whatever we do in Rust.

@ghost
Copy link

ghost commented Sep 11, 2018

Another question: Why are we introducing a new method rather than introducing a wrapper type?
So instead of going with a.total_cmp(b), why not OrdFloat(a).cmp(&OrdFloat(b))?

We already have something similar in the form of std::num::Wrapping, which changes the behavior of integer types. We could likewise add std::num::OrdFloat, which changes the behavior of floating point types.

Wrapper types also make sorting floats easy: v.sort_by_key(OrdFloat).

There's another similar wrapper type: std::cmp::Reverse, which changes the behavior of comparison for any type. Using Reverse for sorting looks very similar: v.sort_by_key(Reverse)

It seems to me wrapper types already became the established pattern for that kind of thing. Is there something I'm missing?

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 11, 2018

@stjepang

Is there a reason why we're not relying on LLVM to compare floats rather than fiddling with bitwise operations? Is it perhaps inconsistent with the exact ordering we'd like to guarantee?

As far as I know LLVM does not provide this operation. The fcmp IR instruction implements the usual (partial) comparison operators, not totalOrder.

Edit: I also don't know of any hardware that provides dedicated support for totalOrder, so I don't think there's any reason to expect an LLVM intrinsic to lead to better codegen.

Another question: Why are we introducing a new method rather than introducing a wrapper type?

I don't have an answer to that. I don't really have an opinion on the API either, I'll leave that to libs folks.

@scottmcm
Copy link
Member Author

Why are we introducing a new method rather than introducing a wrapper type?

Because I started with totalOrder from the standard, realized it would be more rusty to be -> Ordering instead of -> bool, and didn't think about it any more than that 🙂

I tried a -> impl Ord method as an experiment, but that didn't work because #54283. And I suppose it's less helpful anyway if you want to put it into a struct you want to #[derive(Ord)] since existential type isn't stable.

If there is a type, that means a naming and wrapper-vs-concrete discussions, like the NonNull<> vs NonNullU32 debate, and debating whether OrdFloat(0_i32) should compile...

v.sort_by_key(OrdFloat)

This doesn't work for the same reason that v.sort_by(f32::total_cmp) doesn't work: we don't have function coercions yet, and the constructor & function don't take references.

@alexcrichton
Copy link
Member

FWIW although I'm listed as the reviewer here I'm happy to defer to others in this thread as y'all sound more knowledgeable than I anyway!

@hanna-kruppe
Copy link
Contributor

I'd say r=me wrt the implementation if it gains some comments explaining why the bit fiddling works (we had good answers to that in a discussion GH now marked as "outdated"), but I have no opinion on the open question of how to expose this functionality to users -- newtype, inherent method(s), etc. Feedback from @rust-lang/libs would be welcome.

@ghost
Copy link

ghost commented Sep 24, 2018

@scottmcm

If there is a type, that means a naming and wrapper-vs-concrete discussions, like the NonNull<> vs NonNullU32 debate, and debating whether OrdFloat(0_i32) should compile...

So Wrapping is defined as:

#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default, Hash)]
#[repr(transparent)]
pub struct Wrapping<T>(pub T);

Type T can be any type (the only bound is the implicit T: Sized).
That means even Wrapping(vec!["foo"]) is legal.

There is no NonNull equivalent for Wrapping so I assume there wouldn't be any for OrdFloat either. At least not until we introduce a generic NonNull<_>.

We'd probably define it like this:

#[derive(Clone, Copy, Default, Hash)]
#[repr(transparent)]
pub struct OrdFloat<T>(pub T);

And then we'd add impls of PartialEq, Eq, PartialOrd, Ord, Add, Sub, Neg, AddAssign, and so on, but only for f32 and f64, just like we did Wrapping for integer types.

@TimNN TimNN added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018
@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2018

Ping from triage @scottmcm: It looks like @rkruppe has requested some changes to this PR.

@TimNN
Copy link
Contributor

TimNN commented Oct 23, 2018

Ping from triage @scottmcm: What are your plans for this PR?

@jpetkau
Copy link

jpetkau commented Oct 24, 2018

Ping from triage @scottmcm: What are your plans for this PR?

If @scottmcm doesn't respond, is there a process for someone else to make it happen? (I'd be willing send a new PR that's a copy of this one or depends on this one and just makes the requested doc edits.)

Thanks to rkruppe for the ordering details for the doc comments!
@scottmcm
Copy link
Member Author

Ok, I put in a newtype in for this. Let me know what you think. I left the method largely as a place to put documentation about the order; the examples look terrible using the newtype everywhere.

@@ -462,6 +462,40 @@ impl<T: Ord> Ord for Reverse<T> {
}
}

/// A wrapper newtype for providing a total order on a type that's normally partial.
Copy link
Contributor

Choose a reason for hiding this comment

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

The brief summary seems to indicate that only T: PartialOrds are accepted but the signature does not require that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's completely true, but note that cmp::Reverse also doesn't bound itself.

Copy link
Contributor

@gnzlbg gnzlbg Oct 29, 2018

Choose a reason for hiding this comment

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

Indeed, do you know why that is? Appears to be an oversight, what use is reverse if T does not implement PartialOrd ? - If this was an oversight for Reverse, we don't have to make the same mistake here.

#[derive(Debug, Copy, Clone)]
#[unstable(feature = "float_total_cmp", issue = "55339")]
pub struct Total<T>(#[unstable(feature = "float_total_cmp", issue = "55339")] pub T);

Copy link
Contributor

@gnzlbg gnzlbg Oct 25, 2018

Choose a reason for hiding this comment

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

The PartialOrd/Ord "API" makes the assumption that only a single three-way comparator makes sense for any given type. This API covers the case in which more than one comparator makes sense for a given type, and the one implemented for the type is not total.

This feel like an extremely niche case for such a general API in core::cmp. For example, are we going to cover the opposite case, e.g., by adding Partial<T> as well? What about types with two total orders, where users would like to use the non-default one: are we going to add a Total2<T> to core::cmp to cover that? I don't think adding these kinds of types to core::cmp are a good solution to the problem.

Floats are ubiquitous, and it makes sense to make them easier to use. I think we could add [f32, f64]::total_order(self) -> TotalFloat<Self> methods to the floating-point types, and a TotalFloat type wrapper to core::float (or somewhere else) that implements the float API, but where the default ordering is total instead of partial. That's a smaller and more focused problem to solve.

In std::cmp, either we should try to solve the underlying problem, or provide better duct-tape, e.g., Total<T, Fn(Self, Self)-> Ordering> and Partial<T, Fn(Self,Self)->Option<Ordering>>, such that one can write Total<f32, f32::total_cmp> to select the "default" orderings or similar.


The PartialOrd/Ord provide "the default ordering for a type", but the thing they do not address yet is that partial / total are properties of orderings, not of types, and that multiple orderings with different properties exist for most types.

A less intrusive solution to the problem of adjusting the default ordering of aggregates could be to just use macros to control which ordering is used, e.g., instead of

#[derive(PartialOrd,Ord,PartialEq,Eq)]
struct A {
   x: Total<f32>,
}

one could

#[derive(PartialOrd,Ord,PartialEq,Eq)]
struct A {
   #[Ord = f32::total_cmp, Eq = f32::total_eq]
   x: f32,
}

A better long-term solution for types with multiple orderings in the ecosystem would be to evolve APIs like HashSet from requiring the constraints on the type HashSet<T: Eq + Hash>, which kind of implies that for every T there is only an Eq and Hash impl that makes sense, to a de-coupled API like HashSet<T, O: Eq<T> = <T as Eq> where T: Eq, H: Hash<T> = <T as Hash> where T: Hash> that lets the user decide which ordering / hashing to use when it needs to, while falling back to a default if none are provided.

/// ```
#[unstable(feature = "float_total_cmp", issue = "55339")]
#[inline]
pub fn total_cmp(self, other: f64) -> cmp::Ordering {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be implemented using a macro so that all tests and docs can be shared?

Copy link
Member Author

Choose a reason for hiding this comment

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

Plausibly, but that's a general f32.rs/f64.rs problem not specific to this method -- the same is true of min, max, to_bits, etc -- so I will not be taking action on it in this PR.

Copy link
Contributor

@gnzlbg gnzlbg Oct 29, 2018

Choose a reason for hiding this comment

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

Just because the other methods could be improved does not mean that we have to make the situation ""worse"" with this PR. It would be better to just add a float_total_ord.rs module with a macro to define the method and the tests only once, and use that from f32.rs/f64.rs.

If someone wants to reduce duplication for the other methods, they can do the same, and once we have a couple of files we could move them all to a sub-directory or something, but we should start somewhere.

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

I'm uncomfortable with adding the Total<T> API to std::cmp. I feel that it has not been discussed enough / achieved enough consensus, and would prefer if that would be split into a separate PR or a pre-RFC-like discussion in internals since it is unnecessarily blocking the addition of the total_cmp methods.

The total_cmp methods are very useful even without a Total<T> API (worst case users can write their own Total<T>-like API in their own crates, so not having that right now is not the end of the world). I (and it appears that others) are unsure about the total_cmp API, e.g., because it cannot be used for sort_by(f32::total_cmp) "as is", but nobody has proposed anything better and the current API is already good enough so I think it should be merged as is.

I raised some nitpicks about code duplication, but I leave it up to @scottmcm whether these are to be resolved.

@TimNN TimNN added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2018
@TimNN
Copy link
Contributor

TimNN commented Nov 6, 2018

Ping from triage @alexcrichton: It looks like this PR is now ready for your review.

@alexcrichton
Copy link
Member

This sort of continues to have a very large amount of discussion which doesn't seem to have a clear conclusion. @scottmcm should this be closed until the discussion has been settled?

@scottmcm
Copy link
Member Author

scottmcm commented Nov 7, 2018

I don't have a direct need for this myself, so I don't think I'll expend the energy needed to resolve it.

@icefoxen
Copy link
Contributor

@scottmcm I'm willing to pick this up and trim the API to a fine, poodle-like coiffure if you want. Not sure there's a way to formally hand it off to me but I can give it a go.

@scottmcm
Copy link
Member Author

@icefoxen Please do!

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 27, 2020
… r=sfackler

Implement total_cmp for f32, f64

# Overview
* Implements method `total_cmp` on `f32` and `f64`. This method implements a float comparison that, unlike the standard `partial_cmp`, is total (defined on all values) in accordance to the IEEE 754 (rev 2008) §5.10 `totalOrder` predicate.
* The method has an API similar to `cmp`: `pub fn total_cmp(&self, other: &Self) -> crate::cmp::Ordering { ... }`.
* Implements tests.
* Has documentation.

# Justification for the API
* Total ordering for `f32` and `f64` has been discussed many time before:
  * https://internals.rust-lang.org/t/pre-pre-rfc-range-restricting-wrappers-for-floating-point-types/6701
  * rust-lang/rfcs#1249
  * rust-lang#53938
  * rust-lang#5585
* The lack of total ordering leads to frequent complaints, especially from people new to Rust.
  * This is an ergonomics issue that needs to be addressed.
  * However, the default behaviour of implementing only `PartialOrd` is intentional, as relaxing it might lead to correctness issues.
* Most earlier implementations and discussions have been focusing on a wrapper type that implements trait `Ord`. Such a wrapper type is, however not easy to add because of the large API surface added.
* As a minimal step that hopefully proves uncontroversial, we can implement a stand-alone method `total_cmp` on floating point types.
  * I expect adding such methods should be uncontroversial because...
    * Similar methods on `f32` and `f64` would be warranted even in case stdlib would provide a wrapper type that implements `Ord` some day.
    * It implements functionality that is standardised. (IEEE 754, 2008 rev. §5.10 Note, that the 2019 revision relaxes the ordering. The way we do ordering in this method conforms to the stricter 2008 standard.)
* With stdlib APIs such as `slice::sort_by` and `slice::binary_search_by` that allow users to provide a custom ordering criterion, providing additional helper methods is a minimal way of adding ordering functionality.
  * Not also does it allow easily using aforementioned APIs, it also provides an easy and well-tested primitive for the users and library authors to implement an `Ord`-implementing wrapper, if needed.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 27, 2020
… r=sfackler

Implement total_cmp for f32, f64

# Overview
* Implements method `total_cmp` on `f32` and `f64`. This method implements a float comparison that, unlike the standard `partial_cmp`, is total (defined on all values) in accordance to the IEEE 754 (rev 2008) §5.10 `totalOrder` predicate.
* The method has an API similar to `cmp`: `pub fn total_cmp(&self, other: &Self) -> crate::cmp::Ordering { ... }`.
* Implements tests.
* Has documentation.

# Justification for the API
* Total ordering for `f32` and `f64` has been discussed many time before:
  * https://internals.rust-lang.org/t/pre-pre-rfc-range-restricting-wrappers-for-floating-point-types/6701
  * rust-lang/rfcs#1249
  * rust-lang#53938
  * rust-lang#5585
* The lack of total ordering leads to frequent complaints, especially from people new to Rust.
  * This is an ergonomics issue that needs to be addressed.
  * However, the default behaviour of implementing only `PartialOrd` is intentional, as relaxing it might lead to correctness issues.
* Most earlier implementations and discussions have been focusing on a wrapper type that implements trait `Ord`. Such a wrapper type is, however not easy to add because of the large API surface added.
* As a minimal step that hopefully proves uncontroversial, we can implement a stand-alone method `total_cmp` on floating point types.
  * I expect adding such methods should be uncontroversial because...
    * Similar methods on `f32` and `f64` would be warranted even in case stdlib would provide a wrapper type that implements `Ord` some day.
    * It implements functionality that is standardised. (IEEE 754, 2008 rev. §5.10 Note, that the 2019 revision relaxes the ordering. The way we do ordering in this method conforms to the stricter 2008 standard.)
* With stdlib APIs such as `slice::sort_by` and `slice::binary_search_by` that allow users to provide a custom ordering criterion, providing additional helper methods is a minimal way of adding ordering functionality.
  * Not also does it allow easily using aforementioned APIs, it also provides an easy and well-tested primitive for the users and library authors to implement an `Ord`-implementing wrapper, if needed.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 29, 2020
… r=sfackler

Implement total_cmp for f32, f64

# Overview
* Implements method `total_cmp` on `f32` and `f64`. This method implements a float comparison that, unlike the standard `partial_cmp`, is total (defined on all values) in accordance to the IEEE 754 (rev 2008) §5.10 `totalOrder` predicate.
* The method has an API similar to `cmp`: `pub fn total_cmp(&self, other: &Self) -> crate::cmp::Ordering { ... }`.
* Implements tests.
* Has documentation.

# Justification for the API
* Total ordering for `f32` and `f64` has been discussed many time before:
  * https://internals.rust-lang.org/t/pre-pre-rfc-range-restricting-wrappers-for-floating-point-types/6701
  * rust-lang/rfcs#1249
  * rust-lang#53938
  * rust-lang#5585
* The lack of total ordering leads to frequent complaints, especially from people new to Rust.
  * This is an ergonomics issue that needs to be addressed.
  * However, the default behaviour of implementing only `PartialOrd` is intentional, as relaxing it might lead to correctness issues.
* Most earlier implementations and discussions have been focusing on a wrapper type that implements trait `Ord`. Such a wrapper type is, however not easy to add because of the large API surface added.
* As a minimal step that hopefully proves uncontroversial, we can implement a stand-alone method `total_cmp` on floating point types.
  * I expect adding such methods should be uncontroversial because...
    * Similar methods on `f32` and `f64` would be warranted even in case stdlib would provide a wrapper type that implements `Ord` some day.
    * It implements functionality that is standardised. (IEEE 754, 2008 rev. §5.10 Note, that the 2019 revision relaxes the ordering. The way we do ordering in this method conforms to the stricter 2008 standard.)
* With stdlib APIs such as `slice::sort_by` and `slice::binary_search_by` that allow users to provide a custom ordering criterion, providing additional helper methods is a minimal way of adding ordering functionality.
  * Not also does it allow easily using aforementioned APIs, it also provides an easy and well-tested primitive for the users and library authors to implement an `Ord`-implementing wrapper, if needed.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 29, 2020
… r=sfackler

Implement total_cmp for f32, f64

# Overview
* Implements method `total_cmp` on `f32` and `f64`. This method implements a float comparison that, unlike the standard `partial_cmp`, is total (defined on all values) in accordance to the IEEE 754 (rev 2008) §5.10 `totalOrder` predicate.
* The method has an API similar to `cmp`: `pub fn total_cmp(&self, other: &Self) -> crate::cmp::Ordering { ... }`.
* Implements tests.
* Has documentation.

# Justification for the API
* Total ordering for `f32` and `f64` has been discussed many time before:
  * https://internals.rust-lang.org/t/pre-pre-rfc-range-restricting-wrappers-for-floating-point-types/6701
  * rust-lang/rfcs#1249
  * rust-lang#53938
  * rust-lang#5585
* The lack of total ordering leads to frequent complaints, especially from people new to Rust.
  * This is an ergonomics issue that needs to be addressed.
  * However, the default behaviour of implementing only `PartialOrd` is intentional, as relaxing it might lead to correctness issues.
* Most earlier implementations and discussions have been focusing on a wrapper type that implements trait `Ord`. Such a wrapper type is, however not easy to add because of the large API surface added.
* As a minimal step that hopefully proves uncontroversial, we can implement a stand-alone method `total_cmp` on floating point types.
  * I expect adding such methods should be uncontroversial because...
    * Similar methods on `f32` and `f64` would be warranted even in case stdlib would provide a wrapper type that implements `Ord` some day.
    * It implements functionality that is standardised. (IEEE 754, 2008 rev. §5.10 Note, that the 2019 revision relaxes the ordering. The way we do ordering in this method conforms to the stricter 2008 standard.)
* With stdlib APIs such as `slice::sort_by` and `slice::binary_search_by` that allow users to provide a custom ordering criterion, providing additional helper methods is a minimal way of adding ordering functionality.
  * Not also does it allow easily using aforementioned APIs, it also provides an easy and well-tested primitive for the users and library authors to implement an `Ord`-implementing wrapper, if needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.