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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 29, 2017
Merged

cJSON: Fix print_number to print significant digits of doubles #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fc9bc0
cJSON: Fix print_number to print significant digits of doubles
DeboraG Apr 20, 2017
a69ad22
Remove test for negative zero, as -0 is acceptable output
DeboraG Apr 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 11 additions & 44 deletions cJSON.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,37 +410,16 @@ static void update_offset(printbuffer * const buffer)
buffer->offset += strlen((const char*)buffer_pointer);
}

/* Removes trailing zeroes from the end of a printed number */
static int trim_trailing_zeroes(const unsigned char * const number, int length, const unsigned char decimal_point)
{
if ((number == NULL) || (length <= 0))
{
return -1;
}

while ((length > 0) && (number[length - 1] == '0'))
{
length--;
}
if ((length > 0) && (number[length - 1] == decimal_point))
{
/* remove trailing decimal_point */
length--;
}

return length;
}

/* Render the number nicely from the given item into a string. */
static cJSON_bool print_number(const cJSON * const item, printbuffer * const output_buffer, const internal_hooks * const hooks)
{
unsigned char *output_pointer = NULL;
double d = item->valuedouble;
int length = 0;
size_t i = 0;
cJSON_bool trim_zeroes = true; /* should zeroes at the end be removed? */
unsigned char number_buffer[64]; /* temporary buffer to print the number into */
unsigned char number_buffer[26]; /* temporary buffer to print the number into */
unsigned char decimal_point = get_decimal_point();
double test;

if (output_buffer == NULL)
{
Expand All @@ -452,20 +431,17 @@ 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))
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Author

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);
    }

Copy link
Collaborator

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.

{
/* integer */
length = sprintf((char*)number_buffer, "%.0f", d);
trim_zeroes = false; /* don't remove zeroes for "big integers" */
}
else if ((fabs(d) < 1.0e-6) || (fabs(d) > 1.0e9))
{
length = sprintf((char*)number_buffer, "%e", d);
trim_zeroes = false; /* don't remove zeroes in engineering notation */
}
else
{
length = sprintf((char*)number_buffer, "%f", d);
/* 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);
}
}

/* sprintf failed or buffer overrun occured */
Expand All @@ -474,15 +450,6 @@ static cJSON_bool print_number(const cJSON * const item, printbuffer * const out
return false;
}

if (trim_zeroes)
{
length = trim_trailing_zeroes(number_buffer, length, decimal_point);
if (length <= 0)
{
return false;
}
}

/* reserve appropriate space in the output */
output_pointer = ensure(output_buffer, (size_t)length, hooks);
if (output_pointer == NULL)
Expand Down
24 changes: 7 additions & 17 deletions tests/print_number.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,19 @@ static void print_number_should_print_positive_integers(void)
static void print_number_should_print_positive_reals(void)
{
assert_print_number("0.123", 0.123);
assert_print_number("1.000000e-09", 10e-10);
assert_print_number("1e-09", 10e-10);
assert_print_number("1000000000000", 10e11);
assert_print_number("1.230000e+129", 123e+127);
assert_print_number("0", 123e-128); /* TODO: Maybe this shouldn't be 0 */
assert_print_number("1.23e+129", 123e+127);
assert_print_number("1.23e-126", 123e-128);
}

static void print_number_should_print_negative_reals(void)
{
assert_print_number("-0.0123", -0.0123);
assert_print_number("-1.000000e-09", -10e-10);
assert_print_number("-1000000000000000000000", -10e20);
assert_print_number("-1.230000e+129", -123e+127);
assert_print_number("-1.230000e-126", -123e-128);
assert_print_number("-1e-09", -10e-10);
assert_print_number("-1e+21", -10e20);
assert_print_number("-1.23e+129", -123e+127);
assert_print_number("-1.23e-126", -123e-128);
}

static void print_number_should_print_non_number(void)
Expand All @@ -87,15 +87,6 @@ static void print_number_should_print_non_number(void)
/* assert_print_number("null", -INFTY); */
}

static void trim_trailing_zeroes_should_trim_trailing_zeroes(void)
{
TEST_ASSERT_EQUAL_INT(2, trim_trailing_zeroes((const unsigned char*)"10.00", (int)(sizeof("10.00") - 1), '.'));
TEST_ASSERT_EQUAL_INT(0, trim_trailing_zeroes((const unsigned char*)".00", (int)(sizeof(".00") - 1), '.'));
TEST_ASSERT_EQUAL_INT(0, trim_trailing_zeroes((const unsigned char*)"00", (int)(sizeof("00") - 1), '.'));
TEST_ASSERT_EQUAL_INT(-1, trim_trailing_zeroes(NULL, 10, '.'));
TEST_ASSERT_EQUAL_INT(-1, trim_trailing_zeroes((const unsigned char*)"", 0, '.'));
}

int main(void)
{
/* initialize cJSON item */
Expand All @@ -107,7 +98,6 @@ int main(void)
RUN_TEST(print_number_should_print_positive_reals);
RUN_TEST(print_number_should_print_negative_reals);
RUN_TEST(print_number_should_print_non_number);
RUN_TEST(trim_trailing_zeroes_should_trim_trailing_zeroes);

return UNITY_END();
}