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

Implement total_cmp for f32, f64 #72568

Merged
merged 7 commits into from
May 29, 2020

Conversation

golddranks
Copy link
Contributor

@golddranks golddranks commented May 25, 2020

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:
  • 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.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 May 25, 2020
@golddranks
Copy link
Contributor Author

golddranks commented May 25, 2020

Just a note: I'm not entirely sure, but I realized that we might have some architectures (old MIPS chips?) in Tier 2 support that do not exactly conform to the 2008 revision, although they do conform to the relaxed 2019 revision. The problem is that these platforms consider the "quiet" bit in NaN reversed, so on these platforms the order between qNaN and sNaN would be reversed. This requires decision about whether to rather support 2019 instead of 2008, or document that the stricter 2008 is supported on elsewhere but on these rare non-conforming systems. I think that this is not a major problem, just something to decide before stabilising.

@joshtriplett
Copy link
Member

r? @sfackler

@sfackler
Copy link
Member

What change was made in 754-2019 specifically?

src/libcore/num/f32.rs Outdated Show resolved Hide resolved
src/libcore/num/f32.rs Outdated Show resolved Hide resolved
src/libcore/num/f32.rs Outdated Show resolved Hide resolved
golddranks and others added 2 commits May 26, 2020 02:44
Co-authored-by: bluss <bluss@users.noreply.github.com>
Co-authored-by: bluss <bluss@users.noreply.github.com>
@golddranks
Copy link
Contributor Author

golddranks commented May 25, 2020

@sfackler The way the order phrased in 2008 revision is as follows:
スクリーンショット 2020-05-26 2 30 57

I don't currently have access to the text of 2019 revision, but according to these "background documents" http://grouper.ieee.org/groups/msc/ANSI_IEEE-Std-754-2019/background/ it seems that the requirement "lesser payload, when regarded as an integer, orders below greater payload for +NaN, reverse for -NaN." was removed:

The relaxed ordering of NaNs by the 5.10 totalOrder operation is another case where an implementation that conforms to 754-2019 might not conform to 754-2008. Conformance to both could be obtained by satisfying the stricter 754-2008 requirement d)3)iii) "lesser payload, when regarded as an integer, orders below greater payload for +NaN, reverse for -NaN."

Reading that again, that might or might not still mean that ordering between qNaN and sNan is required by 2019. I don't know. If somebody has access to the text, please enlighten me.

@golddranks
Copy link
Contributor Author

Allright, I got my hands to it using... internet magic.
スクリーンショット 2020-05-26 3 10 49

So, the requirement for -qNaN < -sNaN < +sNaN < +qNaN hasn't gone away. As for the hardware support; according to Wikipedia:

For IEEE 754-2008 conformance, the meaning of the signaling/quiet bit in recent MIPS processors is now configurable via the NAN2008 field of the FCSR register. This support is optional in MIPS Release 3 and required in Release 5.[11]

/// ```
#[unstable(feature = "total_cmp", issue = "none")]
#[inline]
pub fn total_cmp(&self, other: &Self) -> crate::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.

Is this implementation sourced from somewhere else? Might be nice to link to that for easy reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is from scratch but the idea was adapted from an earlier thread here: rust-lang/rfcs#1249 (comment)

src/libcore/num/f64.rs Outdated Show resolved Hide resolved
@golddranks
Copy link
Contributor Author

golddranks commented May 25, 2020

Oops, sorry. I touched an unrelated submodule accidentally. I don't want to mess other state up, so rebased and force-pushed.

@sfackler
Copy link
Member

sfackler commented May 25, 2020

Can you make a tracking issue and reference it in the stability annotations? LGTM otherwise; thanks for the extensive tests!

@golddranks
Copy link
Contributor Author

@sfackler Done!

@sfackler
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented May 27, 2020

📌 Commit 66da735 has been approved by sfackler

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 27, 2020
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#72310 (Add Peekable::next_if)
 - rust-lang#72383 (Suggest using std::mem::drop function instead of explicit destructor call)
 - rust-lang#72398 (SocketAddr and friends now correctly pad its content)
 - rust-lang#72465 (Warn about unused captured variables)
 - rust-lang#72568 (Implement total_cmp for f32, f64)
 - rust-lang#72572 (Add some regression tests)
 - rust-lang#72591 (librustc_middle: Rename upvar_list to closure_captures)
 - rust-lang#72701 (Fix grammar in liballoc raw_vec)
 - rust-lang#72731 (Add missing empty line in E0619 explanation)

Failed merges:

r? @ghost
@bors bors merged commit c09f0eb into rust-lang:master May 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2020
…r=Mark-Simulacrum

Run standard library unit tests without optimizations in `nopt` CI jobs

This was discussed in rust-lang#73288 as a way to catch similar issues in the future. This builds an unoptimized standard library with the bootstrap compiler and runs the unit tests. This takes about 2 minutes on my laptop.

I confirmed that this method works locally, although there may be a better way of implementing it. It would be better to use the stage 2 compiler instead of the bootstrap one.

Notably, there are currently four `libstd` unit tests that fail in debug mode on `i686-unkown-linux-gnu` (a tier one target):

```
failures:
    f32::tests::test_float_bits_conv
    f32::tests::test_total_cmp
    f64::tests::test_float_bits_conv
    f64::tests::test_total_cmp
```

These are the tests that prompted rust-lang#73288 as well as the ones added in rust-lang#72568, which is currently broken due to rust-lang#73328.
@KodrAus KodrAus mentioned this pull request Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants