cyb70289 commented on a change in pull request #11720:
URL: https://github.com/apache/arrow/pull/11720#discussion_r752247894
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -247,12 +247,14 @@ struct Multiply {
// Multiplication of 16 bit integer types implicitly promotes to signed 32
bit
// integer. However, some inputs may nevertheless overflow (which triggers
undefined
- // behaviour). Therefore we first cast to 32 bit unsigned integers where
overflow is
- // well defined.
+ // behaviour). One could first cast to 32 bit unsigned integers where
overflow is
+ // well defined, but this is only reasonable if both numbers are positive or
+ // both numbers are negative. It may be better for the user to check for an
overflow
+ // or have a slow safe computation in which the program checks for an
overflow
template <typename T, typename Arg0, typename Arg1>
static constexpr enable_if_same<T, int16_t, T> Call(KernelContext*, int16_t
left,
int16_t right, Status*) {
- return static_cast<uint32_t>(left) * static_cast<uint32_t>(right);
+ return static_cast<int32_t>(left) * static_cast<int32_t>(right);
Review comment:
Looks to me `uint32` or`int32` doesn't matter here.
Actually I think the even the casting to 32 bit integer is not necessary:
`int16 * int16` won't overflow an `int32`.
Only `uint16 * uint16` may overflow `int32` and trigger UB.
--
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]
