Update int_roundings methods from feedback by jhpratt · Pull Request #95359 · rust-lang/rust (original) (raw)

@joshtriplett My thoughts being mentioned are probably those in #88581 (comment)

I actually agree that it would be completely reasonable for next_multiple_of in release mode to panic for division by zero, and wrap for the addition in things like u8::MAX.next_multiple_of(2).

The problem for me comes in the checked_ one. We already see people confused by checked_shl, thinking that u32::MAX.checked_shl(1) would return None because of the overflow, but no, it's not checking that part. I thus worry that checked_next_multiple_of is checking too many things, making it confusing. Or if one day we have saturating_next_multiple_of -- which seems like it'd also be quite reasonable -- what does it do for rhs == 0? Saturating the "next" part I understand, but I have no idea how to saturate the / 0 part. So they could both panic for zero, but that would probably be bad for the https://rust-lang.github.io/rust-clippy/master/index.html#integer_arithmetic folks who are expecting that the checked_ method isn't going to panic.

Taking nonzero types as the RHS for next_multiple_of immediately solves all those questions, which makes it worth it to me -- especially since the ergonomic imperfection is an existing one that we all (AFAIK) agree we want to improve in general anyway, and is no worse than ones that already exist like "I have a u32 I want to use to index my array". (If you're fine with a panic for division-by-zero, .try_into().unwrap() is always available.)

All that, however, I think applies only to the nextmultipleof methods. I would be perfectly fine with div_floor and div_ceil to continue to take rhs: Self. Any checking these is mostly about the zero, with the only exceptions being the rare ones like / -1 that already exist for normal division. I also think the consistency with Div::div and the existing checked_div is far more important for those two than for the no-existing-precedent-in-std next_multiple_of.

including impls on NonZero types that return NonZero types where appropriate

I'll note that we have a bunch of these where appropriate -- for example, checked/saturating add/mul/pow are there for the unsigned ones, BitOr for all of them, abs on the signed ones, etc. They just don't have the "wrap in release" versions, since nearly everything (except BitOr) can wrap to zero in at least one situation.

EDIT: To make a concrete proposal, I suggest that next_multiple_of keeps the NonZero types as in this PR, but that div_* switch back to the non-NonZero types. (And we keep the overflow handing from this PR, as in your proposal.)