[PATCH] D46209: [Support] Make JSON handle doubles and int64s losslessly

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

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.



================
Comment at: lib/Support/JSON.cpp:347
+    // strtoll returns long long >= 64 bits, so check it's in range too.
+    auto I = strtoll(S.c_str(), &End, 10);
+    if (End == S.end() && I >= std::numeric_limits<int64_t>::min() &&
----------------
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>`.


================
Comment at: unittests/Support/JSONTest.cpp:268
+          1.5 * double{0x8000000000000000},
+      },
+  };
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D46209