Add various arithmetic operations #39
Conversation
Small step towards #32
- Implement `wrapping_add`, `wrapping_sub`, `wrapping_mul`, `wrapping_div`, `wrapping_shl`, `wrapping_shr` - In debug builds, `<<` (`Shl`, `ShlAssign`) and `>>` (`Shr`, `ShrAssign`) now bounds-check the shift amount using the same semantics as built-in shifts. For example, shifting a u5 by 5 or more bits will now panic as expected.
src/lib.rs
Outdated
| pub const fn saturating_mul(self, rhs: Self) -> Self { | ||
| let product = if BITS << 1 <= (core::mem::size_of::<$type>() << 3) { | ||
| // We have half the bits (e.g. u4 * u4) of the base type, so we can't overflow the base type | ||
| self.value * rhs.value |
There was a problem hiding this comment.
This does not have to go through the “saturated” logic below, does it?
There was a problem hiding this comment.
Also, don’t you want to use wrapping_mul to avoid redundant checks in debug?
(Same up in shl/shr)?
There was a problem hiding this comment.
For the first question: Yes it does. An u4 is backed by an u8. u4 * u4 can't overflow u8 (hence we skip the first check), but it can still overflow an u4, so we need the second check.
For the second part: I guess - I didn't care too much about debug foot print, but changing to wrapping_mul shouldn't hurt
There was a problem hiding this comment.
For shl/shr: I decided to use << and >> as I'd expect that they perform better on cpus that don't by default do wrapping_shl. Slight hit in Debug builds on "normal" cpus, but optimal code everywhere in Release build
| type Output = UInt<T, BITS>; | ||
|
|
||
| fn mul(self, rhs: Self) -> Self::Output { | ||
| let product = self.value * rhs.value; |
There was a problem hiding this comment.
This has another overflow check in debug, right? We need it, but should have add a comment that there is another check that we can’t avoid?
| // Integer division can only make the value smaller. And as the result is same type as | ||
| // Self, there's no need to range-check or mask | ||
| Self { | ||
| value: self.value / rhs.value, |
There was a problem hiding this comment.
Does the compiler know this though? Should we use an unchecked version to guarantee we avoid checks?
There was a problem hiding this comment.
There's unfortunately not a Trait for wrapping_div in regular Rust, so we can't express the generic in this way (num_traits has that, but that's an optional feature in this crate)
There was a problem hiding this comment.
I don't think it matters too much though as I don't think the compiler checks anything here (what would it even test?)
No description provided.