zhjwpku commented on code in PR #755:
URL: https://github.com/apache/iceberg-cpp/pull/755#discussion_r3437237037
##########
src/iceberg/util/truncate_util.h:
##########
@@ -84,7 +86,15 @@ class ICEBERG_EXPORT TruncateUtils {
template <typename T>
requires std::is_same_v<T, int32_t> || std::is_same_v<T, int64_t>
static inline T TruncateInteger(T v, int32_t W) {
- return v - (((v % W) + W) % W);
Review Comment:
I tried this on Compiler Explorer. For `W > INT32_MAX / 2`, `(v % W) + W`
can overflow, causing the remainder to become negative, which violates the spec
that the remainder must be positive.
For values close to `std::numeric_limits<int64_t>::min()`, our current
implementation produces the same result as the modified version, but it triggers
`runtime error: signed integer overflow`
when compiled with `-fsanitize=undefined`.
I think Java has a similar issue with the first `int32_t` overflow case due
to wraparound arithmetic, no idea if it suffers from the `signed integer
overflow` problem that C++ has(maybe not).
If we change this, I'd suggest keep `v - (((v % W) + W) % W)` as a comment
and clarify why the current form.
I agree this is worth discussing on the dev mailing list.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
