Skip to content
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

Fix coercion of floats to integer for %d printf specifier #37512

Merged
merged 2 commits into from
Sep 10, 2020
Merged

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Sep 10, 2020

Fixes #37507. The issue here was the calling of trunc instead of
round when coercing AbstractFloat arguments to Integer. Note this
throws a scary warning and has undefined behavior in C, but it looks
like legacy Printf code supported this, so we can try to do the right
thing here. Previously, Printf called the grisu equivalent of
Ryu.writefixed(x, precision=0) for float arguments, but as far as I
can tell, calling round(x) should accomplish the same thing.

Fixes #37507. The issue here was the calling of `trunc` instead of
`round` when coercing `AbstractFloat` arguments to `Integer`. Note this
throws a scary warning and has undefined behavior in C, but it looks
like legacy Printf code supported this, so we can try to do the right
thing here. Previously, Printf called the grisu equivalent of
`Ryu.writefixed(x, precision=0)` for float arguments, but as far as I
can tell, calling `round(x)` should accomplish the same thing.
@quinnj quinnj mentioned this pull request Sep 10, 2020
@quinnj
Copy link
Member Author

quinnj commented Sep 10, 2020

but as far as I can tell, calling round(x) should accomplish the same thing.

As noted in this comment, there is a subtle difference between the Grisu & Ryu algorithms in that the former rounds .5 upwards (RoundNearestTiesUp), while the latter rounds ties to even (RoundNearest).

@quinnj quinnj merged commit 0d7dc96 into master Sep 10, 2020
@quinnj quinnj deleted the jq/37507 branch September 10, 2020 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in @sprintf
1 participant