Skip to content

u64::saturating_div is useless #122821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ijackson opened this issue Mar 21, 2024 · 10 comments
Open

u64::saturating_div is useless #122821

ijackson opened this issue Mar 21, 2024 · 10 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ijackson
Copy link
Contributor

pub const fn saturating_div(self, rhs: u64) -> u64

This function will panic if rhs is 0.

This makes no sense. The only way that u64 / u64 could overfllow is if rhs = 0.

The same is true for the other unsigned integer types. The signed types have the excuse that they wouldn't know whether to count division by zero as saturating to -ve or +ve maximum.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 21, 2024
@jieyouxu jieyouxu added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 21, 2024
@tmiasko tmiasko changed the title u64::satuating_div is useless u64::saturating_div is useless Mar 21, 2024
@fmease fmease added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 22, 2024
@Noratrieb
Copy link
Member

I think these functions mostly exist for consistency

@ijackson
Copy link
Contributor Author

That's all very well, but surely this one should return u64::MAX instead of panicking, on division by zero.

@Jules-Bertholet
Copy link
Contributor

"Consistency" isn't usually sufficient justification for having a method. u64::abs doesn't exist, for example.

Perhaps we should just deprecate these?

@ryanavella
Copy link

The signed types have the excuse that they wouldn't know whether to count division by zero as saturating to -ve or +ve maximum.

I don't think this was the only reason. On the PR (#87921) for the std::num::Saturating type, the original implementation guaranteed x / 0 == MAX, but that was changed when @kennytm noted it could give surprising results for 0 / 0.

It's also worth noting that if u64::saturating_div saturated to u64::MAX instead of panicking, that it is somewhat redundant with u64::checked_div (the more general of the two)

@kennytm
Copy link
Member

kennytm commented Mar 25, 2024

The signed types have the excuse that they wouldn't know whether to count division by zero as saturating to -ve or +ve maximum.

No, for signed type saturating_div makes sense because i32::MIN / -1 overflows. It has nothing to do with divide-by-zero (which also panics).

If u64::saturating_div is useless because x / 0 panics then so is u64::wrapping_div and u64::overflowing_div and u64::strict_div which are all equivalent.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Mar 25, 2024

If u64::saturating_div is useless because x / 0 panics

It's not useless just because of this, it's useless because it has identical behavior to u64::div in all cases. No one is proposing deprecating or changing saturating_div for the signed integers. (See my previous analogy to abs)

@kennytm
Copy link
Member

kennytm commented Mar 25, 2024

@Jules-Bertholet All of u64::wrapping_div and u64::strict_div are identical to u64::div and u64::overflowing_div is just (x/y, false).

How is a non-existing function like u64::abs relevant? The consistency of u64::saturating_div is with u64::saturating_{add,sub,mul,...}, not i64::saturating_div.

(speaking of which there should also have been u64::unchecked_div and i64::unchecked_div which are currently missing)

@Jules-Bertholet
Copy link
Contributor

All of u64::wrapping_div and u64::strict_div are identical to u64::div, and u64::overflowing_div is just (x/y, false).

Then IMO these should be deprecated also.

@AaronKutch
Copy link
Contributor

No it is not useless, consistency in integer methods is extremely useful for macro writers. The only issue is that saturating_div possibly should have used the RISC-V convention where division by zero results in iX::MIN if the dividend is negative, 0 if zero, and iX::MAX if positive.

@Noratrieb
Copy link
Member

Yeah, having this consistent integer methods makes writing macros easier (and it makes writing the standard library implementation macros easier :3), and they don't hurt. I think this should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants