arrow-buffer: i256: implement ilog via i256 crate#9453
arrow-buffer: i256: implement ilog via i256 crate#9453theirix wants to merge 4 commits intoapache:mainfrom
Conversation
arrow-buffer/src/bigint/mod.rs
Outdated
| /// Returns `None` if `self` is less than or equal to zero, or if `base` is less than 2. | ||
| #[inline] | ||
| pub fn checked_ilog(self, base: i256) -> Option<u32> { | ||
| if self <= Self::ZERO || base < i256::from(2) { |
There was a problem hiding this comment.
Since these invariants are checked by the call to ExtI256::checked_ilog anyway, what's the point of restating the logic here? The to_ext_i256 calls should be no-ops anyway.
There was a problem hiding this comment.
The reason was to place checks closer to the caller. For procmacros of i256, it's less clear from the source code, but the documentation explicitly lists the required checks, and we repeat them in a docstring. Removed extra code.
| /// Returns `None` if `self` is less than or equal to zero. | ||
| #[inline] | ||
| pub fn checked_ilog10(self) -> Option<u32> { | ||
| // Switch to I256::checked_ilog10 when I256 switches MSRV |
There was a problem hiding this comment.
Why can't it be used now?
There was a problem hiding this comment.
checked_ilog10 is literally commented out in i256 code: Alexhuszagh/i256-rs@85c0bb1/src/int/checked.rs#L102
There was a problem hiding this comment.
Ah, I only checked checked_ilog2. My mistake :)
arrow-buffer/src/bigint/mod.rs
Outdated
| /// Returns `None` if `self` is less than or equal to zero. | ||
| #[inline] | ||
| pub fn checked_ilog2(self) -> Option<u32> { | ||
| // Switch to I256::checked_ilog2 when I256 switches MSRV |
There was a problem hiding this comment.
Same as above: why not now?
There was a problem hiding this comment.
For checked_ilog2 - it exists, updated the call.
arrow-buffer/src/bigint/mod.rs
Outdated
| // log10 should be 76 or 77 | ||
| let result = large.checked_ilog(i256::from(10)); | ||
| assert!(result.is_some()); | ||
| assert!(result.unwrap() >= 76 && result.unwrap() <= 77); |
There was a problem hiding this comment.
Why can't an assert_eq! be used here? Surely the result is deterministic?
2 participants
Which issue does this PR close?
Rationale for this change
Implementation of integer logarithms via
i256crate. There is no matchingnum_traitstrait, but this implementation provides a good motivation for such a trait.What changes are included in this PR?
i256crateAre these changes tested?
Are there any user-facing changes?