Repository navigation
Fix printf and seq %f padding past 1000 zeros and panics on zero - #13460
Conversation
Merging this PR will improve performance by 9.07%
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
|
Please fix the job issue |
|
GNU testsuite comparison: |
| decimal_digits(bd, precision) | ||
| } | ||
|
|
||
| /// Render `bd` in decimal notation with exactly `precision` fractional digits. |
There was a problem hiding this comment.
are you sure these long comments are valuable ?
There was a problem hiding this comment.
Trimmed them, kept only why each branch is there. Same in the tests.
* tests/printf/printf.sh: Ensure a large field width or precision produces fully padded output, and that a precision beyond 1000 digits is not truncated. * tests/seq/seq-precision.sh: Ensure a large format precision is fully honored. Link: uutils/coreutils#13460 Link: #325
There was a problem hiding this comment.
Pull request overview
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.
This PR fixes printf/seq aborts when formatting with very large field widths or precisions by avoiding Rust formatting panics and by bypassing bigdecimal’s Display padding limits for %f.
Changes:
- Add chunked padding writer utilities to avoid
std::fmtpanics for large widths. - Rework decimal
%fformatting to assemble digits directly (fixes >1000 fractional zero padding and large precision panics). - Add regression tests for large width/precision and
bigdecimalpadding behavior inprintfandseq.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/by-util/test_seq.rs | Adds a regression test for large seq -f precision formatting. |
| tests/by-util/test_printf.rs | Adds regression tests for large width/precision and %f padding edge cases. |
| src/uucore/src/lib/features/format/spec.rs | Switches width padding implementation away from std::fmt to chunked padding writes. |
| src/uucore/src/lib/features/format/num_format.rs | Implements manual decimal digit rendering and replaces std::fmt padding with chunked padding writes. |
| src/uucore/src/lib/features/format/mod.rs | Introduces a shared write_padding helper for chunked padding output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fn write_padding(mut writer: impl Write, pad: u8, count: usize) -> std::io::Result<()> { | ||
| let chunk = [pad; 64]; | ||
| let mut remaining = count; | ||
| while remaining > 0 { | ||
| let n = remaining.min(chunk.len()); | ||
| writer.write_all(&chunk[..n])?; | ||
| remaining -= n; | ||
| } | ||
| Ok(()) | ||
| } |
| fn write_padding(mut writer: impl Write, pad: u8, count: usize) -> std::io::Result<()> { | ||
| let chunk = [pad; 64]; |
| let mut out = String::with_capacity(digits.len() + precision + 2); | ||
| if scale <= 0 { | ||
| // An integer: `digits` followed by `-scale` zeros, except for zero itself. | ||
| out.push_str(&digits); | ||
| if digits != "0" { | ||
| out.extend(std::iter::repeat_n('0', -scale as usize)); | ||
| } | ||
| if precision > 0 { | ||
| out.push('.'); | ||
| out.extend(std::iter::repeat_n('0', precision)); | ||
| } |
|
Merged main. |
| /// Write `count` copies of `pad` to `writer`, in fixed-size chunks. | ||
| /// | ||
| /// `write!(writer, "{s:>width$}")` panics past a width of `u16::MAX`. | ||
| fn write_padding(mut writer: impl Write, pad: u8, count: usize) -> std::io::Result<()> { | ||
| let chunk = [pad; 64]; | ||
| let mut remaining = count; | ||
| while remaining > 0 { | ||
| let n = remaining.min(chunk.len()); | ||
| writer.write_all(&chunk[..n])?; | ||
| remaining -= n; | ||
| } | ||
| Ok(()) | ||
| } |
bigdecimal stops padding past 1000 zeros and goes to exponent notation. Write integer digits and the fraction padding ourselves. Zero with a precision above 65535 still panicked in %e, %a and %#g. Build those zeros directly, and use the uppercase prefix for %A.
aa5b79f to
833fab9
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved allocation and large-precision correctness issues remain.
Review effort: Lite
Findings: 1
Open (6)
Bound large precision allocations before formatting · Newwrite_paddingwrites in 64-byte chunks, which can result in ~15kwrite_allcalls for the… In thescale <= 0(integer) branch, the output can grow by-scaleadditional zeros, but the… A 64-byte chunk size will result in manywrite_allcalls for large widths/precisions (e.g., tens… Thewrite_paddingsignature takeswriterby value (impl Write), which encourages callers to… Add seq regression tests for zero and large precision formats · New
| return String::from("0"); | ||
| } | ||
| let mut s = String::from("0."); | ||
| s.extend(std::iter::repeat_n('0', precision)); |
| fn test_zero_precision_above_formatter_limit() { | ||
| let zeros = "0".repeat(70000); | ||
| new_ucmd!() | ||
| .args(&["%.70000e", "0"]) | ||
| .succeeds() | ||
| .stdout_only(format!("0.{zeros}e+00")); |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Moderate formatting correctness and resource-safety issues remain unresolved.
Review effort: Lite
Findings: 1
Open (6)
Bound large precision allocations before formattingwrite_paddingwrites in 64-byte chunks, which can result in ~15kwrite_allcalls for the… In thescale <= 0(integer) branch, the output can grow by-scaleadditional zeros, but the… A 64-byte chunk size will result in manywrite_allcalls for large widths/precisions (e.g., tens… Thewrite_paddingsignature takeswriterby value (impl Write), which encourages callers to… Add seq regression tests for zero and large precision formats
| } else if let Some(s) = integer_digits(bd) { | ||
| s + "." | ||
| } else { | ||
| return format!("{bd:.0}"); |
There was a problem hiding this comment.
here we silently drop the requested precision (and the .), is that intended? if so, could you please add a short comment
There was a problem hiding this comment.
It only kicks in past MAX_FORMAT_WIDTH zeros (%.2f 1e2000000), where main already printed 1e+2000000 and GNU prints inf with an error, so I kept that rather than writing out all the digits. Added a comment in a7ab2b1.
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
Review effort: Lite
Findings: 2
Open (9)
Whenprecision > MAX_FMT_PRECISIONand the number has more fractional digits than… · New Bound large precision allocations before formattinginteger_digitsclaims to cap output toMAX_FORMAT_WIDTHdigits, but it only constrains the… · Newwrite_paddingwrites in 64-byte chunks, which can result in ~15kwrite_allcalls for the… In thescale <= 0(integer) branch, the output can grow by-scaleadditional zeros, but the… A 64-byte chunk size will result in manywrite_allcalls for large widths/precisions (e.g., tens… Thewrite_paddingsignature takeswriterby value (impl Write), which encourages callers to… In theForceDecimal::Yesbranch,format!(\"{s}.\")creates a new allocation even thoughsis… · New Add seq regression tests for zero and large precision formats
| let digits = usize::try_from(bd.fractional_digit_count().max(0)).unwrap_or(usize::MAX); | ||
| let written = digits.min(MAX_FMT_PRECISION); | ||
| if precision > written { | ||
| let mut s = if written > 0 { | ||
| format!("{bd:.written$}") | ||
| } else if let Some(s) = integer_digits(bd) { | ||
| s + "." | ||
| } else { | ||
| // Past `MAX_FORMAT_WIDTH` zeros, print exponent form and skip the precision and `.`. | ||
| return format!("{bd:.0}"); | ||
| }; | ||
| s.extend(std::iter::repeat_n('0', precision - written)); | ||
| return s; | ||
| } |
| .filter(|zeros| *zeros <= super::MAX_FORMAT_WIDTH)?; | ||
| let mut s = bi.to_str_radix(10); |
| let s = integer_digits(bd).unwrap_or_else(|| format!("{bd:.0}")); | ||
| return if force_decimal == ForceDecimal::Yes { | ||
| format!("{s}.") | ||
| } else { | ||
| s | ||
| }; |
A value with more than u16::MAX fractional digits was formatted to u16::MAX digits and then padded with zeros, dropping the real digits. Round the BigDecimal to the precision ourselves (half to even, only when that drops digits) and write its digits out instead of going through format!, which panics past u16::MAX. Follow-up to uutils#13460.



printf '%.1500f' 1printed1, andprintf '%f' 1e1001printed1e+1001. bigdecimal stops zero-padding past 1000 digits and falls back to exponent notation, so %f now writes out integer digits and pads the fraction itself.Zero still aborted with a precision above 65535 in
%e,%aand%#g, for exampleprintf '%.70000e' 0. Those print their zeros directly now, and%Aof zero gets the uppercase0Xprefix it was missing.The width and large
%fprecision panics were fixed on main by #13857, c22ee24 and 2f675af, so I dropped my version of those and rebased on top.Fixes #12708