alamb commented on code in PR #9418:
URL: https://github.com/apache/arrow-rs/pull/9418#discussion_r3343892694
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -807,6 +807,55 @@ impl Shr<u8> for i256 {
}
}
+impl WrappingShl for i256 {
+ #[inline]
+ fn wrapping_shl(&self, rhs: u32) -> i256 {
+ // Mask the higher-order bits
Review Comment:
I can see in the code that `rhs as u8` masks the high order bits but what
was not clear to me was "why"
I think this would be much clearer if it provided the why , something like
```suggestion
// limit shift to 256 (max valid shift for i256)
```
(same below)
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -807,6 +807,55 @@ impl Shr<u8> for i256 {
}
}
+impl WrappingShl for i256 {
+ #[inline]
+ fn wrapping_shl(&self, rhs: u32) -> i256 {
+ // Mask the higher-order bits
+ (*self).shl(rhs as u8)
+ }
+}
+
+impl WrappingShr for i256 {
+ #[inline]
+ fn wrapping_shr(&self, rhs: u32) -> i256 {
+ // Masks the higher-order bits
+ (*self).shr(rhs as u8)
+ }
+}
+
+// Define Shl<T> and Shr<T> for specified integer types using
+// an existing Shl<u8> and Shr<u8> implementation
+macro_rules! define_standard_shift {
+ // Handle multiple types
+ ($trait_name:ident, $method:ident, [$($t:ty),+]) => {
+ $(define_standard_shift!($trait_name, $method, $t);)+
+ };
+ // Handle single type
+ ($trait_name:ident, $method:ident, $t:ty) => {
+ impl $trait_name<$t> for i256 {
+ type Output = i256;
+
+ #[inline]
+ fn $method(self, rhs: $t) -> Self::Output {
+ let rhs = u8::try_from(rhs).expect("rhs overflow for shift
{rhs}");
+ // Other possible overflows are handled by Shl<u8>
implementation
+ self.$method(rhs as u8)
Review Comment:
since rhs is converted to u8 above, this cast seems redundant
##########
arrow-buffer/src/bigint/mod.rs:
##########
@@ -807,6 +807,55 @@ impl Shr<u8> for i256 {
}
}
+impl WrappingShl for i256 {
+ #[inline]
+ fn wrapping_shl(&self, rhs: u32) -> i256 {
+ // Mask the higher-order bits
+ (*self).shl(rhs as u8)
+ }
+}
+
+impl WrappingShr for i256 {
+ #[inline]
+ fn wrapping_shr(&self, rhs: u32) -> i256 {
+ // Masks the higher-order bits
+ (*self).shr(rhs as u8)
+ }
+}
+
+// Define Shl<T> and Shr<T> for specified integer types using
+// an existing Shl<u8> and Shr<u8> implementation
+macro_rules! define_standard_shift {
+ // Handle multiple types
+ ($trait_name:ident, $method:ident, [$($t:ty),+]) => {
+ $(define_standard_shift!($trait_name, $method, $t);)+
+ };
+ // Handle single type
+ ($trait_name:ident, $method:ident, $t:ty) => {
+ impl $trait_name<$t> for i256 {
+ type Output = i256;
+
+ #[inline]
+ fn $method(self, rhs: $t) -> Self::Output {
+ let rhs = u8::try_from(rhs).expect("rhs overflow for shift
{rhs}");
Review Comment:
the panic message isn't interpolated -- so `rhs overflow for shift {rhs}`
will print literally here. I recommend just changing it to "rhs overflow for
shift"
--
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]
