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

TotalOrder trait for floating point numbers #295

Merged
merged 8 commits into from Oct 27, 2023

Conversation

andrewjradcliffe
Copy link
Contributor

Define an orthogonal trait which corresponds to the totalOrder predicate the IEEE 754 (2008 revision) floating point standard.

In order to maintain coherence, the bounds on TotalOrder should most likely be TotalOrder: Float (or TotalOrder: FloatCore). Without type constraints, TotalOrder could be defined on, well, anything. Though slightly ugly, one way to deal with this is to define two traits, TotalOrderCore: FloatCore and TotalOrder: Float. On the other hand, Inv has no such constraints (due to the possibility of a meaningful implementation on rational numbers). If the crate designers could weigh in on whether to introduce constraints, that would be helpful.

Resolves: issue#256

Define an orthogonal trait which corresponds to the `totalOrder`
predicate the IEEE 754 (2008 revision) floating point standard.

In order to maintain coherence, the bounds on `TotalOrder` should most
likely be `TotalOrder: Float` (or `TotalOrder: FloatCore`).  Without
type constraints, `TotalOrder` could be defined on, well, anything.
Though slightly ugly, one way to deal with this is to define two
traits, `TotalOrderCore: FloatCore` and `TotalOrder: Float`.  On the
other hand, `Inv` has no such constraints (due to the possibility of a
meaningful implementation on rational numbers).
src/lib.rs Outdated Show resolved Hide resolved
impl TotalOrder for $T {
#[inline]
fn total_cmp(&self, other: &Self) -> std::cmp::Ordering {
Self::total_cmp(&self, other)
Copy link
Member

Choose a reason for hiding this comment

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

We're going to need a fallback implementation for Rust < 1.62, or else this errors as unconditional recursion. I can add that myself if you're not sure how to go about that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what sort of cfg tricks could be used to conditionally provide a fallback based on language version. Or maybe cfg is not the answer? I will learn from you.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've pushed that and a few other fixups -- please review in return!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the answer is autocfg... no surprise it has 200 million downloads. Very interesting.

Excellent idea to replace std::cmp::Ordering with core::cmp::Ordering, so that this PR is compatible with no_std by default.

Looks good to me.

Tangentially, thank you for the opportunity to contribute; I learned several useful things.

@cuviper
Copy link
Member

cuviper commented Oct 27, 2023

Thanks!

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2023
295: `TotalOrder` trait for floating point numbers r=cuviper a=andrewjradcliffe

Define an orthogonal trait which corresponds to the `totalOrder` predicate the IEEE 754 (2008 revision) floating point standard.

In order to maintain coherence, the bounds on `TotalOrder` should most likely be `TotalOrder: Float` (or `TotalOrder: FloatCore`).  Without type constraints, `TotalOrder` could be defined on, well, anything. Though slightly ugly, one way to deal with this is to define two traits, `TotalOrderCore: FloatCore` and `TotalOrder: Float`.  On the other hand, `Inv` has no such constraints (due to the possibility of a meaningful implementation on rational numbers). If the crate designers could weigh in on whether to introduce constraints, that would be helpful.

Resolves: [issue#256](#256)

Co-authored-by: Andrew Radcliffe <andrewjradcliffe@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 27, 2023

Build failed:

@cuviper
Copy link
Member

cuviper commented Oct 27, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 27, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 61d9a1b into rust-num:master Oct 27, 2023
4 checks passed
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.

[feature request] total_cmp for num_traits::Float
2 participants