-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More robust non-integer conversions for %d Printf specifier #37554
Conversation
Fixes #37552. As pointed out by Jameson, the non-integer conversion code for the `%d` specifier handling code wasn't super robust; I wasn't sure how robust it really needed to be, since it seems a bit sketchy to me for users to be relying on Printf conversion behavior of non-integers with `%d` anyway; C even throws a really scary warning if you pass non-integer for `%d`. But anyways, this isn't that much more machinery and puts back support for printf formatting of `Rational` with `%d` anyway. But a note to users, I'd strongly suggest doing your own conversion, `trunc`, `round`, whatever yourself to ensure you fully understand what's going on if you really need to use the `%d` with non-integer arguments. As for the issue reported, it's note quite as severe a use-case since they're just trying to represent a _huge_ integer via `Float64`, which seems decently reasonable.
@@ -262,11 +262,17 @@ end | |||
end | |||
|
|||
# integers | |||
toint(x) = x | |||
toint(x::Rational) = Integer(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need round
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just matching old printf behavior here:
julia> @sprintf("%d", 3//2)
ERROR: InexactError: Integer(3//2)
Stacktrace:
[1] Integer at ./rational.jl:109 [inlined]
[2] decode_dec at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Printf/src/Printf.jl:855 [inlined]
[3] decode_dec(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Rational{Int64}, ::String, ::Int64, ::Int64, ::Char, ::Array{UInt8,1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Printf/src/Printf.jl:841
[4] top-level scope at REPL[4]:1
julia> @sprintf("%d", 3//1)
"3"
But I could see the case that we should call round
first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it just seemed like the direction we have taken is to integerify any number (admittedly a bit weird -- would be simpler to force the user to do it for %d
). Perhaps the fallback should just be round(Int, x)
instead of a no-op?
toint(x::Rational) = Integer(x) | ||
toint(x::AbstractFloat) = x > typemax(Int128) ? | ||
BigInt(round(x)) : x > typemax(Int64) ? | ||
Int128(round(x)) : Int64(round(x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this so type-unstable seems bad for both the compiler and runtime performance, as well as making it more complex. What did we do before that this wasn't a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we just called the grisu equivalent of Ryu.writefixed
with precision 0
. Let me play around and see if we can do that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, much faster to do that approach: #37601
Alternative fix to #37554. As Jameson noted on that PR, the type instability made the code pretty ugly and slow. An alternative solution is to switch the `%d` format specifier to `%0f`, i.e. a fixed precision of 0 for `AbstractFloat` arguments. The code here is indeed much cleaner and more performant: several times faster than old Printf.
Alternative fix to #37554. As Jameson noted on that PR, the type instability made the code pretty ugly and slow. An alternative solution is to switch the `%d` format specifier to `%0f`, i.e. a fixed precision of 0 for `AbstractFloat` arguments. The code here is indeed much cleaner and more performant: several times faster than old Printf.
Fixes #37552. As pointed out by Jameson, the non-integer conversion code
for the
%d
specifier handling code wasn't super robust; I wasn't surehow robust it really needed to be, since it seems a bit sketchy to me
for users to be relying on Printf conversion behavior of non-integers
with
%d
anyway; C even throws a really scary warning if you passnon-integer for
%d
. But anyways, this isn't that much more machineryand puts back support for printf formatting of
Rational
with%d
anyway. But a note to users, I'd strongly suggest doing your own
conversion,
trunc
,round
, whatever yourself to ensure you fullyunderstand what's going on if you really need to use the
%d
withnon-integer arguments.
As for the issue reported, it's note quite as severe a use-case since
they're just trying to represent a huge integer via
Float64
, whichseems decently reasonable.