-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
cJSON: Fix print_number to print significant digits of doubles #153
Conversation
Thanks a lot for looking into this. This has definitely been one of the pain points with cJSON, I have even started playing around with the Grisu2 algorithm. I probably won't be able to review this before wednesday or thursday next week, but I'm definitely looking forward to it. |
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.
Good work. I have just two small remarks, after that it is ready for merging.
cJSON.c
Outdated
{ | ||
length = sprintf((char*)number_buffer, "%e", d); | ||
trim_zeroes = false; /* don't remove zeroes in engineering notation */ | ||
length = sprintf((char*)number_buffer, "0"); |
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.
Why print -0
as 0
? RFC7159 allows negative zero and it can be represented by doubles internally, so we can as well print it as is.
@@ -452,20 +431,22 @@ static cJSON_bool print_number(const cJSON * const item, printbuffer * const out | |||
{ | |||
length = sprintf((char*)number_buffer, "null"); | |||
} | |||
else if ((fabs(floor(d) - d) <= DBL_EPSILON) && (fabs(d) < 1.0e60)) |
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 think it still makes sense to print integers without engineering notation, as long as they can be printed with full precision.
So instead of removing this integer handling completely, 1e60 should be replaced with 2^53.
I'm just not sure if this integer detection with DBL_EPSILON
is correct, this might have to be fixed.
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.
Update: modf
can be used to test if it is an integer.
If you don't want to tackle this, I can do this myself after the PR is merged.
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.
Since -0 is allowed, there is no need to change it to 0.
As for printing integers without engineering notation as long as they can be printed with full precision, the code as in the pull request mostly handles this. 2^53 is 9,007,199,254,740,992, a little less than 10^17 (17 decimal places). The %1.17g format already prints 17 decimal places.
9007199254740991, 9007199254740992, and 9007199254740994 all print as 16-digit integers as desired. (9007199254740993 is not representable exactly as a double.)
The problem is with the test-print to 15 digits (%1.15g). It causes integers ending in 0 that require more than 15 decimal digits to print in exponential notation.
100,000,000,000,000 prints as desired, with 15 decimal digits: 100000000000000
(I manually added comma separators to help myself count.)
1,000,000,000,000,000 prints as 1e+15, but we want 1000000000000000
10,000,000,000,000,000 prints as 1e+16, but we want 10000000000000000
100,000,000,000,000,000 prints as 1e+17, but that is OK according to your proposed rule because it is greater than 2^53.
The solution is to qualify the test-print. I will have to think more carefully about the value of LIMIT_FOR_TEST_PRINTING - is it 1e+15? I'll have more time to think tomorrow night.
if (fabs(d) < LIMIT_FOR_TEST_PRINTING)
{
/* Try 15 decimal places of precision to avoid nonsignificant nonzero digits */
length = sprintf((char*)number_buffer, "%1.15g", d);
/* Check whether the original double can be recovered */
if ((sscanf((char*)number_buffer, "%lg", &test) != 1) || ((double)test != d))
{
/* If not, print with 17 decimal places of precision */
length = sprintf((char*)number_buffer, "%1.17g", d);
}
}
else
{
/* Print with 17 decimal places of precision */
length = sprintf((char*)number_buffer, "%1.17g", d);
}
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 don't think it works that way because a lot of doubles are bigger than LIMIT_FOR_TEST_PRINTING
but should still be recoverable with just 15 decimal places of precision.
Anyway, if the printing of Integers in engineering notation is only a special case for numbers bigger than 10^15 and smaller than -10^15 (or somewhere around that) I can live with it. I though this happened to more numbers than just that.
In this case let's go for simplicity and keep the code as it is. Just remove the special case for negative zero and it can be merged.
Thank you very much. I will merge this now. |
This makes changes to the print precision in cJSON print_number() and corresponding changes to the expected results in tests/print_number.c. See issue 152.