The documentation contradicts the meaning of abs_sub

[maxbla]

/// The positive difference of two numbers.
///
/// Returns `zero` if the number is less than or equal to `other`, otherwise the difference
/// between `self` and `other` is returned.
fn abs_sub(&self, other: &Self) -> Self;

When I hear abs_sub, I expect it to be |a-b|. At the very least, I think that the abs_sub function could be created as some composition or combination of the abs function and the sub function, but it appears that abs_sub is more of a saturating sub or a clamped sub (i.e. max(0, a-b))

Furthermore, the documentation seems to contradict itself -- "The positive difference of two numbers" is not the same as "zero if the number is less than equal to other". For example, with a=3 and b=5, I would expect the positive difference to mean the difference a - b = 3 - 5 = -2, but now positive = 2.

And finally, if a Type implements Num (which includes NumOps, which includes Sub) and it implements Signed, then abs_sub will always have the same form of

    fn abs_sub(&self, other: &Self) -> Self {
        tmp = self - other;
        if tmp.is_negative {
            Self::zero()
        }
        tmp
    }

I suggest either documenting abs_sub as meaning |a-b|, and providing an implementation or removing it entirely.

[cuviper]

FWIW, the behavior is like C's fdim, but indeed the name is not good. They also describe this as the "positive difference" though.

Returns the positive difference between x and y, that is, if x>y, returns x-y, otherwise (if x≤y), returns +0.

Maybe we should just deprecate it (rust-num/num#365), as the corresponding method in std was already deprecated (rust-lang/rust#33664). Removing it or changing its behavior would be a breaking change.

[maxbla]

I think we should deprecate it. I was not aware of std doing so, but if they did and no one complained (I think?) we could probably get away with the same. Also it's nice to be more similar to std.

[cuviper]

Ideally we would deprecate it while making it not a required method for the trait anymore. There's not a great way to write it generically though, since we don't have PartialOrd here. Your sketched implementation risks overflow, which will silently appear positive in release mode.

[maxbla]

My sketch can overflow, but that is essentially how it works for isize, i8, i16, i32 and i64.
From the source code:

#[inline]
fn abs_sub(&self, other: &$t) -> $t {
    if *self <= *other { 0 } else { *self - *other }
}

[cuviper]

The existing code can overflow for self > other, like i32::MAX.abs_sub(-1), although yours will see that overflowed negative and clamp to 0. That's sort of OK, as it's wrong either way. But yours may also overflow cases that should correctly return 0, like i32::MIN.abs_sub(1).

[maxbla]

Okay, I see what you mean. So if I changed that sketch to be the same as the current implementation, like

fn abs_sub(&self, other: &Self) -> Self {
    self <= other {
        Self::zero()
    } else {
        *self - *other
    }
}

Then you would be okay with deprecating it and providing a built-in implementation?

[cuviper]

That would be fine, but we can't use <= without PartialOrd.

[cuviper]

Anyway, having a default impl is ideal for deprecation, but not mandatory. We could still deprecate it as-is, just with the annoying fact that anyone implementing the trait still has to write this method.

[ocstl]

The existing code can overflow for self > other, like i32::MAX.abs_sub(-1), although yours will see that overflowed negative and clamp to 0. That's sort of OK, as it's wrong either way. But yours may also overflow cases that should correctly return 0, like i32::MIN.abs_sub(1).

PartialOrd would be great, but I think there is a way around it. We have two possible overflows: positive and negative.

In order to (possibly) have a positive overflow, self must be non-negative and other strictly negative. For example, 0.abs_sub(i32::MIN) will overflow. But since not all of these cases lead to overflow, I don't see any great way to deal with it. But I believe the main issue is with negative overflows.

In order to (possibly) have a negative overflow, self must be strictly negative and other strictly positive. Again, not all cases lead to overflow. But, necessarily, self < other, which means the result is 0. Since Signed offers the is_positive and is_negative methods, a simple guard clause could allow a default implementation without PartialOrd.

Alternately, since it is only required that self <= other (when both are zero, the difference is 0), the guard clause can be set to a self that is non-positive and an other that is non-negative.

That being said, NaNs create some issues (no surprise there). But the guard clause can be expanded with equality tests (self == self && other == other) to avoid returning 0 when dealing with NaNs. We can use equality tests, since Signed is bound by Num, which is bound by PartialEq.