Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Implement total_cmp for f32, f64#72568

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bors merged 7 commits intorust-lang:masterfromgolddranks:add_total_cmp_to_floats
May 29, 2020

Conversation

@golddranks
Copy link
Contributor

@golddranksgolddranks commentedMay 25, 2020
edited
Loading

Overview

  • Implements methodtotal_cmp onf32 andf64. This method implements a float comparison that, unlike the standardpartial_cmp, is total (defined on all values) in accordance to the IEEE 754 (rev 2008) §5.10totalOrder predicate.
  • The method has an API similar tocmp:pub fn total_cmp(&self, other: &Self) -> crate::cmp::Ordering { ... }.
  • Implements tests.
  • Has documentation.

Justification for the API

  • Total ordering forf32 andf64 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 onlyPartialOrd is intentional, as relaxing it might lead to correctness issues.
  • Most earlier implementations and discussions have been focusing on a wrapper type that implements traitOrd. 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 methodtotal_cmp on floating point types.
    • I expect adding such methods should be uncontroversial because...
      • Similar methods onf32 andf64 would be warranted even in case stdlib would provide a wrapper type that implementsOrd 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 asslice::sort_by andslice::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 anOrd-implementing wrapper, if needed.

stepancheg, U007D, ugoa, and surban reacted with thumbs up emojijdahlstrom, CGMossa, and GrayJack reacted with hooray emojiyoshuawuyts, sdroege, akofke, dima74, messense, darksv, pmarks, hbina, bluss, Aaron1011, and 20 more reacted with heart emoji
@rust-highfive
Copy link
Contributor

r?@joshtriplett

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

@rust-highfiverust-highfive added the S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelMay 25, 2020
@golddranks
Copy link
ContributorAuthor

golddranks commentedMay 25, 2020
edited
Loading

Just a note: I'm not entirely sure, but I realized that wemight have some architectures (old MIPS chips?) in Tier 2 support that do not exactly conform to the 2008 revision, although theydo 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?

golddranksand others added2 commitsMay 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
ContributorAuthor

golddranks commentedMay 25, 2020
edited
Loading

@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
ContributorAuthor

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]

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
ContributorAuthor

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)

@golddranksgolddranksforce-pushed theadd_total_cmp_to_floats branch from7bd87ff to8bc31ffCompareMay 25, 2020 20:07
@golddranks
Copy link
ContributorAuthor

golddranks commentedMay 25, 2020
edited
Loading

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 commentedMay 25, 2020
edited
Loading

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

@golddranks
Copy link
ContributorAuthor

@sfackler Done!

@sfackler
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

📌 Commit66da735 has been approved bysfackler

@borsbors removed the S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelMay 27, 2020
@borsbors added the S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. labelMay 27, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull requestMay 27, 2020
… r=sfacklerImplement 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 requestMay 27, 2020
… r=sfacklerImplement 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 requestMay 29, 2020
… r=sfacklerImplement 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 requestMay 29, 2020
Rollup of 9 pull requestsSuccessful 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
@borsbors merged commitc09f0eb intorust-lang:masterMay 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull requestAug 14, 2020
…r=Mark-SimulacrumRun standard library unit tests without optimizations in `nopt` CI jobsThis was discussed inrust-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 promptedrust-lang#73288 as well as the ones added inrust-lang#72568, which is currently broken due torust-lang#73328.
@KodrAusKodrAus mentioned this pull requestNov 6, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@llogiqllogiqllogiq left review comments

+2 more reviewers

@sfacklersfacklersfackler left review comments

@blussblussbluss left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sfacklersfackler

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@golddranks@rust-highfive@joshtriplett@sfackler@bors@bluss@llogiq

[8]ページ先頭

©2009-2025 Movatter.jp