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.