Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
What changed in this PR
This PR updates integer truncation logic to avoid signed overflow/UB during truncation, especially around type boundary values.
Changes:
- Reworked
TruncateIntegerto normalize negative remainders and perform subtraction in unsigned space. - Added boundary-focused unit tests for
TruncateIntegerandTruncateLiteral.
| File | Description |
|---|---|
| src/iceberg/util/truncate_util.h | Changes integer truncation implementation to avoid signed overflow UB. |
| src/iceberg/test/truncate_util_test.cc | Adds regression tests covering min/max and large-width boundary behavior. |
Review effort: Lite
| /// behavior when the truncated value is below T's minimum. Conversion back to T | ||
| /// preserves two's-complement wraparound (defined since C++20). |
| if (remainder < 0) { | ||
| remainder += W; | ||
| } | ||
| return static_cast<T>(static_cast<U>(v) - static_cast<U>(remainder)); |
| /// | ||
| /// 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 |
There was a problem hiding this comment.
For widths above roughly 2^30 this returns 0 where Java's truncateInt wraps and returns -2147483646 (v = 2147483645, W = 2147483646) - the right call given Java's own int and long paths disagree there, but worth stating in this doc block rather than only in a test comment.
|
I recall a similar PR (#755), where we agreed to open a discussion on the dev mailing list. I haven't checked whether the Java impl has since fixed this issue. |
No description provided.