This is an archive of the discontinued LLVM Phabricator instance.

[Support] Make JSON handle doubles and int64s losslessly
ClosedPublic

Authored by sammccall on Apr 27 2018, 1:18 PM.

Details

Summary

This patch adds a new "integer" ValueType, and renames Number -> Double.
This allows us to preserve the full precision of int64_t when parsing integers
from the wire, or constructing from an integer.
The API is unchanged, other than giving asInteger() a clearer contract.

In addition, always output doubles with enough precision that parsing will
reconstruct the same double.

Diff Detail

Event Timeline

sammccall created this revision.Apr 27 2018, 1:18 PM
sammccall updated this revision to Diff 144391.Apr 27 2018, 1:32 PM

Clarify docs, add deserializer for int64_t

Other than these nitpicks, this looks good to me. I've rebased my tablegen patch on to it and confirmed that it now handles integers the way I'd like it to, without me having had to change a line of my client code.

lib/Support/JSON.cpp
347

Tiniest nitpick ever: in this function you've explicitly said std::strtod, but used strtoll unqualified. I wasn't able to figure out which of <cstdlib> and <stdlib.h> actually ends up being included by the headers this file asks for, but std::strtoll would probably be safer, just in case it's <cstdlib>.

unittests/Support/JSONTest.cpp
268

You mentioned in D45753 that there might at some point be a need to handle uint64_t as well as int64_t. Given that, it might make sense to include some tests of negative integers now, so that if that does need to be added later, we'll catch any accidental breakage of the negative integer tests?

sammccall marked 2 inline comments as done.Apr 30 2018, 10:33 AM

Thanks for verifying this! (Still waiting for a decision on the master patch, I hope this doesn't block you too long!)

unittests/Support/JSONTest.cpp
268

Done. I *really* hope we never have to support both at once!

sammccall marked an inline comment as done.

Negative number test case, fix std:: qualification.

simon_tatham accepted this revision.May 1 2018, 1:09 AM

LGTM. Thanks!

This revision is now accepted and ready to land.May 1 2018, 1:09 AM
This comment was removed by simon_tatham.

Oops – posted a comment in the wrong browser tab :-( Sorry for the noise, and especially sorry to @nhaehnle who I didn't actually intend to subscribe to this review.

nhaehnle removed a subscriber: nhaehnle.May 30 2018, 6:33 AM
This revision was automatically updated to reflect the committed changes.