Copilot commented on code in PR #1042:
URL: https://github.com/apache/iceberg-cpp/pull/1042#discussion_r4313116328
##########
src/iceberg/util/truncate_util.h:
##########
@@ -82,12 +83,20 @@ class ICEBERG_EXPORT TruncateUtils {
/// \brief Truncate an integer v, either int32_t or int64_t, to v - (v % W).
///
- /// The remainder, v % W, must be positive. For languages where % can
produce negative
- /// values, the correct truncate function is: v - (((v % W) + W) % W)
+ /// W must be positive. Since C++ can produce a negative remainder, add W
only
+ /// when v % W is negative to normalize it to [0, W). This addition cannot
overflow.
+ /// The final subtraction uses unsigned arithmetic to avoid signed-overflow
undefined
+ /// behavior when the truncated value is below T's minimum. Conversion back
to T
+ /// preserves two's-complement wraparound (defined since C++20).
Review Comment:
The statement “preserves two's-complement wraparound (defined since C++20)”
is inaccurate: the standard unsigned-to-signed conversion for out-of-range
values is implementation-defined (and the language still does not universally
mandate two’s-complement). Please reword this comment to reflect the actual
guarantee (or lack thereof), or explicitly state/verify the project’s
assumption (and/or switch to a defined mechanism like `std::bit_cast` if you
want bit-preserving behavior).
##########
src/iceberg/util/truncate_util.h:
##########
@@ -82,12 +83,20 @@ class ICEBERG_EXPORT TruncateUtils {
/// \brief Truncate an integer v, either int32_t or int64_t, to v - (v % W).
///
- /// The remainder, v % W, must be positive. For languages where % can
produce negative
- /// values, the correct truncate function is: v - (((v % W) + W) % W)
+ /// W must be positive. Since C++ can produce a negative remainder, add W
only
+ /// when v % W is negative to normalize it to [0, W). This addition cannot
overflow.
+ /// The final subtraction uses unsigned arithmetic to avoid signed-overflow
undefined
+ /// behavior when the truncated value is below T's minimum. Conversion back
to T
+ /// preserves two's-complement wraparound (defined since C++20).
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);
+ using U = std::make_unsigned_t<T>;
+ T remainder = v % W;
+ if (remainder < 0) {
+ remainder += W;
+ }
+ return static_cast<T>(static_cast<U>(v) - static_cast<U>(remainder));
Review Comment:
`static_cast<T>(unsigned_value)` is implementation-defined when the unsigned
result is not representable in `T` (which is exactly what happens for cases
like `min - remainder`). If the goal is deterministic two’s-complement
wraparound, consider converting back to `T` in a fully specified way (e.g.,
compute `U truncated = ...;` and use `std::bit_cast<T>(truncated)`), or enforce
the required behavior with an explicit compile-time assumption/guard (and
document it). This avoids relying on implementation-defined unsigned-to-signed
narrowing.
--
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]
