split: fix add-overflow in suffix-length calculation - #13754
Open
leeewee wants to merge 1 commit into
Open
Conversation
Suffix::from computed the auto suffix length from `start as u64 + chunks`, which overflows when the numeric/hex suffix start (--numeric-suffixes / --hex-suffixes) or the chunk count (-n) is near u64::MAX, panicking with `attempt to add with overflow` under overflow-checks. Widen the sum to u128 so it can't overflow; it only feeds a log for the digit count, so results are unchanged for every in-range input and the out-of-range case now reports the normal "suffix length needs to be at least N" error.
Merging this PR will degrade performance by 18.31%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | df_with_path |
571.4 µs | 699.6 µs | -18.31% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing leeewee:split-fix-numeric-suffix-add-overflow (383f289) with main (21d4e96)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13749
splitauto-computes the suffix length from the suffix start value(
--numeric-suffixes=N/--hex-suffixes=N) and the chunk count (-n).Suffix::fromdid this asstart as u64 + chunks, with no guard againstthe sum exceeding
u64::MAX. A start nearu64::MAX, or a chunk countnear
u64::MAXwith a nonzero start, overflows the add and panics withattempt to add with overflowunder overflow-checks (exit 134).Fix
Widen the sum to
u128so it can't overflow. The value only feeds alogfor the digit count, so results are identical for every in-range input,
and the out-of-range case now reports the normal
the suffix length needs to be at least Nerror instead of aborting —matching GNU (which accepts such a start only once
-ais large enough).Tests
Adds three regression tests covering both operands and both radixes: a huge
decimal start, a huge hex start, and a huge
-nchunk count. Each panicsbefore the fix (the test build enables overflow-checks) and passes after.