Skip to content

Preserve precision when passing BigDecimal or BigFloat to sprintf %i#15808

Merged
straight-shoota merged 3 commits intocrystal-lang:masterfrom
HertzDevil:feature/bigdecimal-sprintf-i
May 23, 2025
Merged

Preserve precision when passing BigDecimal or BigFloat to sprintf %i#15808
straight-shoota merged 3 commits intocrystal-lang:masterfrom
HertzDevil:feature/bigdecimal-sprintf-i

Conversation

@HertzDevil
Copy link
Contributor

When String::Formatter formats a non-integer for integer specifiers such as %d, it calls #to_i on that value to reuse the implementation for Int32, meaning BigDecimals and BigFloats outside Int32's range will raise an OverflowError. Fortunately, BigInt is already compatible with sprintf, so they could delegate to BigInt instead of Int32.

This may look similar to #15203, but the fix there is not as simple as delegating to #to_big_f, because that also loses precision.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric labels May 21, 2025
@straight-shoota straight-shoota added this to the 1.17.0 milestone May 21, 2025
@straight-shoota straight-shoota merged commit a95371c into crystal-lang:master May 23, 2025
36 checks passed
@HertzDevil HertzDevil deleted the feature/bigdecimal-sprintf-i branch May 23, 2025 09:20
straight-shoota pushed a commit that referenced this pull request May 26, 2025
This is the only type in the standard library where `#to_i32` is already defined but `#to_i` isn't.

This makes `BigRational` suspectible to precision loss when passed to `sprintf` `%i` (see also #15808), since `BigRational#to_i32` currently delegates to `#to_f64` instead of `#to_big_i`. However this is still an improvement over the current situation where `BigRational` isn't supported by `sprintf` at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:numeric

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants