mbutrovich commented on code in PR #11425:
URL: https://github.com/apache/arrow-rs/pull/11425#discussion_r4221433775
##########
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.
I put the sentence back on both methods and added the rounding and `MIN /
-1` behavior in words. `wrapping_div` and `wrapping_rem` now point to
`wrapping_div_rem`.
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -1621,6 +1671,71 @@ mod tests {
}
}
+ fn test_div_rem(n: i256, d: i256) {
Review Comment:
> 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.
`test_div_rem` now checks all six methods and handles a zero divisor. The
Division and Remainder sections of `test_ops` are a single call to it, and the
`continue` in `test_div_rem_powers_of_ten` is gone.
--
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]
