Tracking Issue for int_abs_diff

[orlp]

Feature gate: #![feature(int_abs_diff)]

This is a tracking issue for a method to compute the absolute difference between two integers.

Public API

This adds the method T::abs_diff(other: T) -> U to all built-in Rust integer types, both signed and unsigned, where U is the unsigned equivalent of T (or just T itself it it already was unsigned). This method always computes the mathematical absolute difference between the two numbers correctly, without any risk of overflow. Because the output is an unsigned type the result is always representable.

Steps / History

• Issue: Add a method for computing the absolute difference between unsigned integers #62111
  Note: scope has been expanded to implement abs_diff for all integers, not just unsigned integers.
• Implementation: Added abs_diff for integer types. #88780
• Final comment period (FCP)
• Stabilization PR: Stabilize int_abs_diff in 1.60.0. #93735

Unresolved Questions

• None yet.

[tspiteri]

For signed, I would prefer the name unsigned_abs_diff as it returns a different type, making it consistent with unsigned_abs.

[orlp]

For unsigned_abs a distinction needed to be made with a pre-existing abs function. We don't have to worry about that backwards compatibility here.

[tspiteri]

But I found i64::abs_diff returning u64 very surprising: a method that operates on two numbers to give a new number silently changes the type. The only other i64 method returning u64 is unsigned_abs, and its name clearly indicates that.

There are other methods that never return a negative value, and they still return the same type. For example i64::abs itself returns i64, not u64, even though it can overflow as well on signed (although only for one value).

[shreepads]

Current state:

u32vec.iter().fold(0, |acc, x| acc + (*x as i32 - median as i32).abs() as u32)

Desired state:

u32vec.iter().fold(0, |acc, x| acc + x.abs_diff(median))        // as per experimental API for u32

[MarcelRobitaille]

@shreepads I have done this:

fn abs_diff(a: u32, b: u32) -> u32 {
  if a > b {
    a - b
  } else {
    b - a
  }
}

Somehow this is cleaner to me than casting to signed. I think it's more efficient too? Not as nice as if it were a built-in method, but still cleans up your fold, no?

[rrokkam]

@tspiteri, if you want abs_diff to return an i64, what behavior do you suggest for the case when abs_diff can't fit in an i64? Panic? Change return type to result? I see no appealing options.

(The difference between two i64s can be 64 bits long, and i64 has 63 bits for magnitude)

[tspiteri]

@rrokkam I would expect the same overflow behaviour as abs itself.

[nhamovitz]

(FWIW, because I'm newish to Rust —)

I very much disagree — it doesn't make sense to me to ship an overflowing API when a 100% correct + infallible algorithm exists. I'll concede some case for naming it unsigned_abs_diff to highlight that the type is changing (although IMO that's what the function signature + your tooling is there for), but allowing overflows seems bad.

[shreepads]

@shreepads I have done this:

fn abs_diff(a: u32, b: u32) -> u32 {
  if a > b {
    a - b
  } else {
    b - a
  }
}

Somehow this is cleaner to me than casting to signed. I think it's more efficient too? Not as nice as if it were a built-in method, but still cleans up your fold, no?

Thanks @MarcelRobitaille , yes that is much better and would work till such time as a method is available in the std library - (there is already such a method in the experimental API for u32)...

I was only hacking one of the Advent of Code puzzles (https://adventofcode.com/2021/day/7) so didn't mind the casting/ re-casting..

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[tspiteri]

Has a final decision on the name abs_diff vs unsigned_abs_diff been made? I did have a use case where the input values were pretty small (pixel values with a limited range of 256) but then the output was used in an algorithm that required a larger signed range. In that case overflow was not an issue.

Of course, with no overflow there is no tricky case that requires this method, and (a - b).abs() is basically fine. I only brought this up in the first place because if an abs_diff method is present it kinda leads to using a.abs_diff(b) instead of (a - b).abs(), and then there will be an API signedness inconsistency between abs_diff and abs. Since (a - b).abs() is not too cumbersome, it may not be an issue, as long as a concious decision is made before stabilization.

[rrokkam]

The difference is that (a - b).abs() panics with underflow when b > a, and when b <= a the abs() call does nothing.

a.abs_diff(b) does not panic when b > a.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[tspiteri]

The difference is that (a - b).abs() panics with underflow when b > a, and when b <= a the abs() call does nothing.

This is for unsigned, while I was talking about signed.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[CleanCut]

@tspiteri Reading through this thread, I don't feel like there was a response explaining why your suggestion was not taken. In the spirit of collaboration, I'd like to attempt to fill in that gap according to my understanding (as an observer, not a decision maker) why abs_diff is an appropriate name!

Adding unsigned anywhere in a method name (usually in the suffix) is helpful to distinguish between a pair of analogous functions for both signed and unsigned flavors of the same operation. Otherwise, in general it is redundant to include signedness in the method name. You can see this (implicit?) naming policy if you examine the long list of methods on i32. Every method that includes unsigned in the name has a corresponding method with a signed flavor (which simply omit unsigned rather than containing signed). The rest of the methods do not mention the signedness in the name of the method at all, including the other almost two dozen methods which return unsigned values other than the one you cited (which does have an analogous signed method).

Anyway, just trying to help you see why I believe your naming suggestion wasn't selected. I could be wrong, in which case I welcome clarification. And, of course, you are welcome to continue preferring your own naming suggestion. I just wanted to make sure you felt heard. 😄

[m-ou-se]

Has a final decision on the name abs_diff vs unsigned_abs_diff been made?

Sorry for the delayed response. We did quickly discuss this in a meeting, and concluded that we don't want a signed version of this, so there's no need for unsigned in the name. See @CleanCut's comment, who worded it better than I could have. :)

[JohnTitor]

Triage: the stabilization PR (#93735) has landed 🎉, closing as done.

[catblue88]

very funny, updated all to the recommended latest versions and got:
error[E0658]: use of unstable library feature 'int_abs_diff'
--> ... solana-core-1.11.5/src/serve_repair.rs:649:48
|
649 | let time_diff_ms = timestamp().abs_diff(header.timestamp);
| ^^^^^^^^
|
= note: see issue #89492 #89492 for more information

following compiler advice I got here.. And noticed that for so many months, in spite of "JohnTitor closed this on 23 May" it has been still leaving gory traces...

[JohnTitor]

@catblue88 What's your rustc version? Note that it requires 1.60 or higher to use the methods on stable.

[catblue88]

hi John,
I went out to let some steam off :-)
Yes, in spite of my effort to update to 1.63.0 it was still 1.59.0 I used.
You were right, it builds up now..
Thanks a lot for you lightning fast reaction..