Just realized that Implementations of PartialOrd, and Ord must agree with each other

[DrAlta]

Older version of the docs just said

Implementations of PartialEq, PartialOrd, and Ord must agree with each other. It's easy to accidentally make them disagree by deriving some of the traits and manually implementing others.

The latest says,

Implementations must be consistent with the PartialOrd implementation, and ensure max, min, and clamp are consistent with cmp:

• partial_cmp(a, b) == Some(cmp(a, b)).
• max(a, b) == max_by(a, b, cmp) (ensured by the default implementation).
• min(a, b) == min_by(a, b, cmp) (ensured by the default implementation).
• For a.clamp(min, max), see the method docs (ensured by the default implementation).

So I cut and pasted partail_cmp to cmp and removed the Some()s and rewrote it so that it doesn't use nested match statements while I was adding the checks for NaN

[dnsl48]

Hi @DrAlta, thank you for looking into it!

ensure max, min, and clamp are consistent with cmp

It sounds like we should have a test proving that statement. If it fails, then we know it's inconsistent across both and we can fix them. Would you like to add the test?

There also seem to be some styling issues in the MR. Rustfmt should fix that easily.

[DrAlta]

I was going to turn you down on creating the test, then I got an idea on how to do the tests while writing the reply. I have to go through what I was working on this morning before I completely forget what I was doing

Looks like I didn't do too bad on formatting just rustfmt just removed the ','s at the end of the match cases and split
(GenericFraction::Rational(ref ls, ref l), GenericFraction::Rational(ref rs, ref r)) => { to multiple lines.

[DrAlta]

Ok, I made a function that compares the result of clamp() and cmp().

I just made a test that tries it with 10 random numbers.

lint is failing do to a bug in Clippy, looks like we'll have to wait until https://github.com/rust-lang/rust-clippy/commit/065c6f78e7b4089ad93eafb0c0c478bbfaa00db8 makes in to the CI

[dnsl48]

Thank you for following up on that!

Ok, I made a function that compares the result of clamp() and cmp().

I am not too sure if the test captures the problem, though. It seems to be always passing even without the code changes in this PR. Do you think there is a way of capturing the problem we are trying to fix by introducing the changes in cmp and partial_cmp?

[DrAlta]

It seems to be always passing even without the code changes in this PR.

Oh, I didn't do anything about clamp(). I just did the partial_cmp(a, b) == Some(cmp(a, b)) bit

e.g. assert_eq!(nan.partial_cmp(&one), Some(nan.cmp(&one)));

, the rests are being taken care by the default implementations with are ensured to agree.

I wrote some more test and came up with a better name that foobar for the old one.

[dnsl48]

Thank you, that looks good!