Migrated to f32::total_cmp() function in FloatOrd::cmp()

[nhunds]

Objective

Fixes #5152

Solution

Use total_cmp() function from f32 in cmp() function on FloatOrd

[alice-i-cecile]

Code change looks good to me. @DJMcNab, advice on the hash concern you raised?

[mockersf]

This should be benchmarked too 👍

[DJMcNab]

I don't think the hash concern is relevant here, since the only thing that matters is hash in relation to PartialEq

However, I think the clippy lint we ignore is making a good point - we should also implement PartialOrd in terms of this Ord impl.

[DJMcNab]

Saying that, I'm not convinced PartialEq doesn't have requirements related to the PartialOrd implementation

Basically, we need to check the documentation for all four traits, which I can't right at this moment.

[nhunds]

I think we don't need the PartialEq implementation, but in my opinion it seems like f32 doesn't have a implementation of the Hash trait.

[james7132]

Going to second the PartialOrd implementation. The Ord documentation explicitly states the two must be in sync.

[DJMcNab]

We still need to think about the Hash implementation

At the moment, we either:

• need a test which loops through all f32 values, and checks that the implementations agree. I expect that they won't, unfortunately.
• convert the only usage of this impl (in FontAtlasSet) to use BTreeMap. I suspect for this workload, that would even be faster. This is the path I'd recommend taking

We also need to use a manual PartialOrd impl.

[superdump]

* convert the only usage of this impl (in `FontAtlasSet`) to use `BTreeMap`. I suspect for this workload, that would even be faster. This is the path I'd recommend taking

Why would BTreeMap be faster? It would still be a binary search versus a hash and direct lookup, right?

[DJMcNab]

Based on the using with_capacity(1), I assume that the majority of the cases have very few items.

Additionally, the keys are small, so a binary search should be very fast.

[atlv24]

The same approach taken in #10558 should be used here

[NthTensor]

I'm going to close this out. Looks like this is wanted, but work has stalled out and the approach needs to be changed. Feel free to re-open.