andygrove commented on code in PR #11425:
URL: https://github.com/apache/arrow-rs/pull/11425#discussion_r4212165219
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -572,6 +572,48 @@ impl i256 {
self.div_rem(other).map(|(_, v)| v).ok()
}
+ /// Performs wrapping division and remainder, returning `(quotient,
remainder)`
+ ///
+ /// # Panics
+ ///
+ /// Panics if `other` is zero
+ ///
+ /// # Example
+ /// ```
+ /// # use arrow_buffer::i256;
+ /// let (q, r) = i256::from_i128(-7).wrapping_div_rem(i256::from_i128(2));
+ /// assert_eq!((q, r), (i256::from_i128(-3), i256::from_i128(-1)));
+ ///
+ /// let (q, r) = i256::MIN.wrapping_div_rem(i256::MINUS_ONE);
+ /// assert_eq!((q, r), (i256::MIN, i256::ZERO));
+ /// ```
+ #[inline]
+ pub fn wrapping_div_rem(self, other: Self) -> (Self, Self) {
+ match self.div_rem(other) {
+ Ok(v) => v,
+ Err(DivRemError::DivideByZero) => panic!("attempt to divide by
zero"),
+ Err(_) => (Self::MIN, Self::ZERO),
+ }
+ }
Review Comment:
This is now the third copy of the zero-divisor panic and the `MIN / -1`
fallback, after `wrapping_div` and `wrapping_rem` above. Those two could become
`self.wrapping_div_rem(other).0` and `.1`, which keeps the error handling in
one place and lets `test_wrapping_div_rem_zero_panics` cover the panic for all
three. All three are `#[inline]` and `div_rem` always computes both values, so
it costs nothing at runtime.
Matching `Err(DivRemError::DivideOverflow)` instead of `Err(_)` would also
turn a future `DivRemError` variant into a compile error here instead of a
silent `(MIN, 0)`.
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -572,6 +572,48 @@ impl i256 {
self.div_rem(other).map(|(_, v)| v).ok()
}
+ /// Performs wrapping division and remainder, returning `(quotient,
remainder)`
Review Comment:
The first commit had a sentence saying this does one division where calling
`wrapping_div` and `wrapping_rem` does two. I'd keep it, since it's the main
reason to call these methods. It would also help to say in words what the
examples show: the quotient truncates toward zero, the remainder has the sign
of the dividend, and `MIN / -1` returns `(MIN, 0)` (or `None` from
`checked_div_rem`). A pointer from the `wrapping_div`/`wrapping_rem` docs would
help people find these methods.
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1621,6 +1671,71 @@ mod tests {
}
}
+ fn test_div_rem(n: i256, d: i256) {
+ let bn = BigInt::from_signed_bytes_le(&n.to_le_bytes());
+ let bd = BigInt::from_signed_bytes_le(&d.to_le_bytes());
+
+ // BigInt produces an integer over i256::MAX for i256::MIN / -1
+ let (q, overflow) = i256::from_bigint_with_overflow(bn.clone() /
bd.clone());
+ let (r, _) = i256::from_bigint_with_overflow(bn % bd);
+
+ assert_eq!(n.wrapping_div_rem(d), (q, r), "{n} / {d}");
+ assert_eq!(
+ n.checked_div_rem(d),
+ (!overflow).then_some((q, r)),
+ "{n} / {d}"
+ );
+ }
+
+ #[test]
+ #[cfg_attr(miri, ignore)] // Takes too long
+ fn test_div_rem_powers_of_ten() {
+ let dividends = [
+ i256::ZERO,
+ i256::ONE,
+ i256::MIN,
+ i256::MIN + i256::ONE,
Review Comment:
nit: `i256::MIN + i256::ONE` is `-i256::MAX`, so with the four sign
combinations below it produces exactly the same `(n, d)` pairs as `i256::MAX`
and could be dropped.
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1621,6 +1671,71 @@ mod tests {
}
}
+ fn test_div_rem(n: i256, d: i256) {
Review Comment:
nit: this recomputes the `BigInt` reference values that the Division and
Remainder sections of `test_ops` already compute, and checks the `MIN / -1`
case a second way (the overflow flag here, the explicit `ir == i256::MINUS_ONE
&& il == i256::MIN` check there). If this helper also asserted `wrapping_div`,
`wrapping_rem`, `checked_div` and `checked_rem`, those sections could become a
single `test_div_rem(il, ir)` call, and `test_div_rem_powers_of_ten` would
cover the single-op methods as well.
It could also handle `d == 0` itself, asserting that `checked_div_rem`
returns `None` and returning before the `BigInt` division panics. That would
replace both the if/else in `test_ops` and the `continue` in
`test_div_rem_powers_of_ten`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
