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

create a sensible comparison trait hierarchy #12520

Closed
wants to merge 1 commit into from
Closed

create a sensible comparison trait hierarchy #12520

wants to merge 1 commit into from

Conversation

thestinger
Copy link
Contributor

  • Ord inherits from Eq
  • TotalOrd inherits from TotalEq
  • TotalOrd inherits from Ord
  • TotalEq inherits from Eq

This is a partial implementation of #12517.

@pcwalton
Copy link
Contributor

Can we maybe just merge the traits, making equals a default method on Eq and cmp a non-default method on Ord (and move lt to be a default method)? Then we could remove TotalEq and TotalOrd entirely.

@thestinger
Copy link
Contributor Author

@pcwalton: This pull request is just attempting to clean up the existing situation rather than changing anything, because there was strong opposition to using the total ordering by default for floating point.

@thestinger
Copy link
Contributor Author

After this, I plan on renaming TotalOrd and TotalEq to Ord and Eq. They will be the only traits used by most generic code. The existing Ord and Eq would be called PartialOrd and PartialEq and deriving(Eq, Ord) will implement them automatically.

This improves the current situation by allowing generic code using the total ordering to have the operators available, while also allowing floating point to continue having deriving of partial ordering available. It's not simple, but it's also not more complex than the current situation so I think it's a clear improvement.

@pcwalton
Copy link
Contributor

I agree that this would be an improvement, but why not just merge the two traits (PartialOrd and Ord) instead of having two? Then sorting could use < (partial) while binary search trees could use .cmp() (total), with both being grouped under the Ord trait.

To be clear:

trait Ord {
    fn lt -> bool { self.cmp(other) == Less }
    fn le -> bool { self.cmp(other) != Greater }
    fn gt -> bool { self.cmp(other) == Greater }
    fn ge -> bool { self.cmp(other) != Less }
    fn cmp -> Ordering;
}

trait Eq {
    fn eq -> bool { self.equals(other) }
    fn ne -> bool { !self.eq(other) }
    fn equals -> bool;
}

impl Ord for f32 {
    fn lt -> bool { *self < *other }
    ...
    fn cmp -> Ordering { transmute::<&f32,&i32>(self).cmp(transmute(other)) }
}

impl Eq for f32 {
    fn eq -> bool { *self == *other }
    fn ne -> bool { *self == *other }
    fn equals -> bool { self.cmp(other) == Equal }
}

This solution: (a) provides IEEE 754-compliance; (b) works with hash tables, including using NaNs as keys; (c) supports sorting arrays of floats; (d) has the simplicity of only two traits; (e) works with deriving; (f) "does the right thing" in the vast majority of circumstances, the biggest hazard being people using .cmp() on floats and expecting it to behave like the mathematical notion of floating comparison (but I think there is little we can do about that, since the mathematical notion of less-than in IEEE 754 just ain't total).
I am not opposed to merging this PR as a stopgap.

@glaebhoerl
Copy link
Contributor

@pcwalton If I'm understanding correctly, then because it would mean generic code couldn't use the comparison operator overloads, because they couldn't be assumed to implement a total order, whereas generic code most often requires one. Enabling generic code (over Ord) to use the operators and expect a total order is one of the goals of this PR.

@pcwalton
Copy link
Contributor

Not all generic code should use a total ordering. Consider sorting: that should definitely not use the total ordering on floats, because the fast total ordering on floats is bitwise less-than, not mathematical less-than. I think code that wants a total ordering should never use the overloaded operators, because the overloaded operators suggest that they do the "mathematical" thing, whereas code that wants a total ordering explicitly doesn't want an ordering which corresponds with the mathematical ordering.

@thestinger
Copy link
Contributor Author

@pcwalton: The IEEE754 totalOrder does use a sane mathematical ordering. It considers -0 to be less than +0, and NaN as equal to itself but it doesn't distinguish because different encodings of the same value.

@pcwalton
Copy link
Contributor

It's not fast to implement in software though. I talked to sunfish at lunch and apparently very few people use the IEEE 754 total ordering when they need an ordering for binary trees—they just bitcast to int and use that.

@thestinger
Copy link
Contributor Author

It does seem like it would have a very significant impact on sort performance. I haven't measured though, and our sort function uses cmp to save a pass with sequence types.

a) If x < y, totalOrder(x, y) is true.
b) If x > y, totalOrder(x, y) is false.
c) If x = y:
    1) totalOrder(−0, +0) is true.
    2) totalOrder(+0, −0) is false.
    3) If x and y represent the same floating-point datum:
        i) If x and y have negative sign, totalOrder(x, y) is true if and only if the exponent of x ≥ the exponent of y
        ii) otherwise totalOrder(x, y) is true if and only if the exponent of x ≤ the exponent of y.
d) If x and y are unordered numerically because x or y is NaN:
    1) totalOrder(−NaN, y) is true where −NaN represents a NaN with negative sign bit and y is a floating-point number.
    2) totalOrder(x, +NaN) is true where +NaN represents a NaN with positive sign bit and x is a floating-point number.
    3) If x and y are both NaNs, then totalOrder reflects a total ordering based on:
        i) negative sign orders below positive sign
        ii) signaling orders below quiet for +NaN, reverse for −NaN
        iii) lesser payload, when regarded as an integer, orders below greater payload for +NaN, reverse for −NaN.

Neither signaling NaNs nor quiet NaNs signal an exception. For canonical x and y, totalOrder(x, y) and totalOrder( y, x) are both true if x and y are bitwise identical.

NOTE — totalOrder does not impose a total ordering on all encodings in a format. In particular, it does not distinguish among different encodings of the same floating-point representation, as when one or both encodings are non-canonical.

@bill-myers
Copy link
Contributor

Floats have multiple orderings useful in different cases, so the only reasonable solution seems to be to force the user to explicitly select which ordering they want for floats.

In practice, that means accepting this pull request and adding a bunch of newtypes implementing normal order with NaNs banned, integer order, IEEE 754 total order, normal order except for -nan < -inf, and so on.

BTW, I think IEEE 754 total order can be implemented like this, which shouldn't be terribly inefficient (at least if the float is in memory, and thus doesn't need to be moved from FPU registers to ALU registers):

fn total_order(af: f32, bf: f32) -> Ordering {
    let a: i32 = unsafe {transmute(af)};
    let b: i32 = unsafe {transmute(bf)};
    if (a & b) >= 0 { // both positive, or different signs
        a.cmp(b)
    } else { // both negative
        b.cmp(a)
    }
}

or without the if:

fn total_order(af: f32, bf: f32) -> Ordering {
    let a: i32 = unsafe {transmute(af)};
    let b: i32 = unsafe {transmute(bf)};
    let mask: i32 = (a & b) >> 31; // if both negative, all ones else all zeroes
    (a ^ mask).cmp(b ^ mask);
}

EDIT: fixed, oops, also improved

@huonw
Copy link
Member

huonw commented Feb 25, 2014

let a = af as s32;
let b = bf as s32;

is meant to be transmute::<f32, i32>(af) (etc), right? (i.e. raw bitwise cast rather than numerical cast.)

@brson
Copy link
Contributor

brson commented Feb 27, 2014

I know this doesn't get us to a final solution, but do we want to merge this for the sake of forward progress?

@emberian
Copy link
Member

emberian commented Mar 2, 2014

I think we should.

@brson
Copy link
Contributor

brson commented Mar 6, 2014

Requires rebase.

* `Ord` inherits from `Eq`
* `TotalOrd` inherits from `TotalEq`
* `TotalOrd` inherits from `Ord`
* `TotalEq` inherits from `Eq`

This is a partial implementation of #12517.
bors added a commit that referenced this pull request Mar 8, 2014
* `Ord` inherits from `Eq`
* `TotalOrd` inherits from `TotalEq`
* `TotalOrd` inherits from `Ord`
* `TotalEq` inherits from `Eq`

This is a partial implementation of #12517.
@bors bors closed this Mar 8, 2014
@thestinger thestinger deleted the cmp branch March 8, 2014 06:03
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
Add xFrednet back to the reviewing rotation 🎉

You know what? Having a work-life balance is boring.

I truly enjoyed having a free few weeks, and learned that I need to take a break every once in a while, but not doing reviews feels weird. So, this is me coming back for more :D

---

r? `@ghost`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants