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 cmp() to f32 and f64, with tests #12193

Closed
wants to merge 1 commit into from

Conversation

indirect
Copy link
Contributor

Sometimes, it becomes necessary to find out if one of your floats is
bigger or smaller than another one of your floats. In my case, I had a
list of floats I had calculated, and I wanted to sort them biggest to
smallest. Using sort_by() requires cmp, so I initially wrote my own
fcmp, and then realized that was silly and it should just be built in.

Sometimes, it becomes necessary to find out if one of your floats is
bigger or smaller than another one of your floats. In my case, I had a
list of floats I had calculated, and I wanted to sort them biggest to
smallest. Using `sort_by()` requires cmp, so I initially wrote my own
fcmp, and then realized that was silly and it should just be built in.
@thestinger
Copy link
Contributor

It's not a correct implementation of TotalEq. It requires these invariants:

  • a != b returns the same value as !(a == b)
  • a == a is true
  • a == b implies b == a
  • a == b && b == c implies a == c

The first two requirements are not true for floating point types using this implementation. It's separate from Eq because Eq is implemented for floating point types via the hardware comparison operation. It's also not a correct implementation of TotalOrd for similar reasons.

What should cmp for two NaN values return? At the moment it's saying they are equal. However, the TotalEq implementation is saying they are not equal. It's also treating -0.0 and 0.0 as equal but they are not equivalent, so it's breaking some basic assumptions about equality/ordering.

@thestinger thestinger closed this Feb 11, 2014
@indirect
Copy link
Contributor Author

So the official stance of Rust is that it's not possible to sort floats?

Is this pull so unfixable that it really deserved to be closed without comment beyond "it's wrong sometimes"?

@lilyball
Copy link
Contributor

@indirect I believe that, by definition, IEEE floating-point values cannot possibly implement TotalEq and TotalOrd. The requirements of those traits are incompatible with NaN.

@thestinger
Copy link
Contributor

@indirect: The TotalEq and TotalOrd traits exist because floating point types implement Eq and Ord. It may make sense to do things differently, but incorrect implementations of TotalEq and TotalOrd are not the right solution. They would be no different than Eq and Ord...

@indirect
Copy link
Contributor Author

I gathered "not the right solution". That's a very concise restatement leading to my previous question, which is still unanswered. What is the right way?

I'm somewhat incredulous because, as far as I can tell, your answer seems to be "never want to sort floats".

On Tue, Feb 11, 2014 at 2:54 PM, Daniel Micay notifications@github.com
wrote:

@indirect: The TotalEq and TotalOrd traits exist because floating point types implement Eq and Ord. It may make sense to do things differently, but incorrect implementations of TotalEq and TotalOrd are not the right solution.

Reply to this email directly or view it on GitHub:
#12193 (comment)

@thestinger
Copy link
Contributor

I think we should have Eq and Ord represent a total ordering, and perhaps implement the floating point total ordering predicate to implement them for floating point types. The traditional ordering can be made available through other methods, possibly via a Float trait.

@thestinger
Copy link
Contributor

#10320

JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Aug 30, 2022
fix: Fix panics on GATs involving const generics

This workaround avoids constant crashing of rust analyzer when using GATs with const generics,
even when the const generics are only on the `impl` block.

The workaround treats GATs as non-existing if either itself or the parent has const generics and
removes relevant panicking code-paths.

Fixes rust-lang#11989, fixes rust-lang#12193
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.

3 participants