Should the implementation of `Ord::max` be changed to use `<` instead of `>=`?

libs
1

Currently, Ord::max is defined as

fn max(self, other: Self) -> Self {
    if other >= self { other } else { self }
}

I noticed, that the compiler is not able to optimize away the division with zero check in this scenario:

fn div_test(val_1: usize, val_2: usize) -> usize {
    let val = core::cmp::max(val_2, 1);
    val_1 / val
}

But if we change the implementation of Ord::max to

fn max(self, other: Self) -> Self {
    if self < other { other } else { self }
}

the compiler is able to omit the checks for division with zero.

Link to GodBolt (Link is with -C opt-level=3, same result with -O)

1 Like
2

Try it out in a perf run and see if it is beneficial?

3

IMHO this should be improved in the compiler optimization level, not the lib level. In both cases the same optimization should apply.

4 Likes
4

Note: self < other is not equivalent with other >= self. And there probably is a good reason it’s defined like this. This problem has a precedent, coming from a mistake made by the C++ specification, where min and max have been accidentally defined in an asymmetric manner.

Specifically, if you have a == b, you most often want/expect let (lo, hi) = (min(a, b), max(a, b)) to bind lo to a and hi to b. With this implementation of max(), hi would be bound to a too. This assumption is made, sometimes even unconsciously, in the context of pointer wrangling and unsafe code so IMO it’s best to regard this as a breaking change and keep the symmetry.

4 Likes
5

This is optimized well:

fn div_test(val_1: usize, val_2: usize) -> usize {
    let val = core::cmp::max(1, val_2); // arguments swapped
    val_1 / val
}

Using < instead of >= may only invert which is optimized well and which is not.

10 Likes
6

I guess this is the way to go, however I have no clue how to improve this :anguished:

7

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.