From d7c4c3ec0b367ab4297f71124d305191ec7d344f Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Mon, 9 Jul 2018 12:16:40 +0000 Subject: [PATCH] [Support] Make JSON handle doubles and int64s losslessly 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. Reviewers: simon_tatham Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D46209 llvm-svn: 336541 --- include/llvm/Support/JSON.h | 60 ++++++++++++++++++++++++---------- lib/Support/JSON.cpp | 42 +++++++++++++++--------- unittests/Support/JSONTest.cpp | 60 ++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 33 deletions(-) diff --git a/include/llvm/Support/JSON.h b/include/llvm/Support/JSON.h index 767e25a8dd2..4b009438b71 100644 --- a/include/llvm/Support/JSON.h +++ b/include/llvm/Support/JSON.h @@ -204,7 +204,7 @@ inline bool operator!=(const Array &L, const Array &R) { return !(L == R); } /// Each Value is one of the JSON kinds: /// null (nullptr_t) /// boolean (bool) -/// number (double) +/// number (double or int64) /// string (StringRef) /// array (json::Array) /// object (json::Object) @@ -226,7 +226,7 @@ inline bool operator!=(const Array &L, const Array &R) { return !(L == R); } /// fromJSON(const json::Value&, T&)->bool /// Deserializers are provided for: /// - bool -/// - int +/// - int and int64_t /// - double /// - std::string /// - vector, where T is deserializable @@ -254,6 +254,8 @@ public: enum Kind { Null, Boolean, + /// Number values can store both int64s and doubles at full precision, + /// depending on what they were constructed/parsed from. Number, String, Array, @@ -281,24 +283,36 @@ public: Value(llvm::StringRef V) : Type(T_StringRef) { create(V); } Value(const char *V) : Type(T_StringRef) { create(V); } Value(std::nullptr_t) : Type(T_Null) {} - // Prevent implicit conversions to boolean. - template ::value>::type> + // Boolean (disallow implicit conversions). + // (The last template parameter is a dummy to keep templates distinct.) + template < + typename T, + typename = typename std::enable_if::value>::type, + bool = false> Value(T B) : Type(T_Boolean) { create(B); } - // Numbers: arithmetic types that are not boolean. + // Integers (except boolean). Must be non-narrowing convertible to int64_t. template < typename T, - typename = typename std::enable_if::value>::type, + typename = typename std::enable_if::value>::type, typename = typename std::enable_if::value>::type> - Value(T D) : Type(T_Number) { - create(D); + Value(T I) : Type(T_Integer) { + create(int64_t{I}); + } + // Floating point. Must be non-narrowing convertible to double. + template ::value>::type, + double * = nullptr> + Value(T D) : Type(T_Double) { + create(double{D}); } // Serializable types: with a toJSON(const T&)->Value function, found by ADL. template ::value>> + Value, decltype(toJSON(*(const T *)nullptr))>::value>, + Value * = nullptr> Value(const T &V) : Value(toJSON(V)) {} Value &operator=(const Value &M) { @@ -319,7 +333,8 @@ public: return Null; case T_Boolean: return Boolean; - case T_Number: + case T_Double: + case T_Integer: return Number; case T_String: case T_StringRef: @@ -344,12 +359,17 @@ public: return llvm::None; } llvm::Optional getAsNumber() const { - if (LLVM_LIKELY(Type == T_Number)) + if (LLVM_LIKELY(Type == T_Double)) return as(); + if (LLVM_LIKELY(Type == T_Integer)) + return as(); return llvm::None; } + // Succeeds if the Value is a Number, and exactly representable as int64_t. llvm::Optional getAsInteger() const { - if (LLVM_LIKELY(Type == T_Number)) { + if (LLVM_LIKELY(Type == T_Integer)) + return as(); + if (LLVM_LIKELY(Type == T_Double)) { double D = as(); if (LLVM_LIKELY(std::modf(D, &D) == 0.0 && D >= double(std::numeric_limits::min()) && @@ -407,9 +427,8 @@ private: enum ValueType : char { T_Null, T_Boolean, - // FIXME: splitting Number into Double and Integer would allow us to - // round-trip 64-bit integers. - T_Number, + T_Double, + T_Integer, T_StringRef, T_String, T_Object, @@ -417,7 +436,7 @@ private: }; // All members mutable, see moveFrom(). mutable ValueType Type; - mutable llvm::AlignedCharArrayUnion Union; }; @@ -505,6 +524,13 @@ inline bool fromJSON(const Value &E, int &Out) { } return false; } +inline bool fromJSON(const Value &E, int64_t &Out) { + if (auto S = E.getAsInteger()) { + Out = *S; + return true; + } + return false; +} inline bool fromJSON(const Value &E, double &Out) { if (auto S = E.getAsNumber()) { Out = *S; diff --git a/lib/Support/JSON.cpp b/lib/Support/JSON.cpp index 0371d1ab251..c2025bb2299 100644 --- a/lib/Support/JSON.cpp +++ b/lib/Support/JSON.cpp @@ -104,7 +104,8 @@ void Value::copyFrom(const Value &M) { switch (Type) { case T_Null: case T_Boolean: - case T_Number: + case T_Double: + case T_Integer: memcpy(Union.buffer, M.Union.buffer, sizeof(Union.buffer)); break; case T_StringRef: @@ -127,7 +128,8 @@ void Value::moveFrom(const Value &&M) { switch (Type) { case T_Null: case T_Boolean: - case T_Number: + case T_Double: + case T_Integer: memcpy(Union.buffer, M.Union.buffer, sizeof(Union.buffer)); break; case T_StringRef: @@ -152,7 +154,8 @@ void Value::destroy() { switch (Type) { case T_Null: case T_Boolean: - case T_Number: + case T_Double: + case T_Integer: break; case T_StringRef: as().~StringRef(); @@ -217,7 +220,7 @@ private: } // On invalid syntax, parseX() functions return false and set Err. - bool parseNumber(char First, double &Out); + bool parseNumber(char First, Value &Out); bool parseString(std::string &Out); bool parseUnicode(std::string &Out); bool parseError(const char *Msg); // always returns false @@ -317,25 +320,28 @@ bool Parser::parseValue(Value &Out) { } } default: - if (isNumber(C)) { - double Num; - if (parseNumber(C, Num)) { - Out = Num; - return true; - } else { - return false; - } - } + if (isNumber(C)) + return parseNumber(C, Out); return parseError("Invalid JSON value"); } } -bool Parser::parseNumber(char First, double &Out) { +bool Parser::parseNumber(char First, Value &Out) { + // Read the number into a string. (Must be null-terminated for strto*). SmallString<24> S; S.push_back(First); while (isNumber(peek())) S.push_back(next()); char *End; + // Try first to parse as integer, and if so preserve full 64 bits. + // strtoll returns long long >= 64 bits, so check it's in range too. + auto I = std::strtoll(S.c_str(), &End, 10); + if (End == S.end() && I >= std::numeric_limits::min() && + I <= std::numeric_limits::max()) { + Out = int64_t(I); + return true; + } + // If it's not an integer Out = std::strtod(S.c_str(), &End); return End == S.end() || parseError("Invalid JSON value (number?)"); } @@ -558,8 +564,12 @@ void llvm::json::Value::print(raw_ostream &OS, const Indenter &I) const { case T_Boolean: OS << (as() ? "true" : "false"); break; - case T_Number: - OS << format("%g", as()); + case T_Double: + OS << format("%.*g", std::numeric_limits::max_digits10, + as()); + break; + case T_Integer: + OS << as(); break; case T_StringRef: quote(OS, as()); diff --git a/unittests/Support/JSONTest.cpp b/unittests/Support/JSONTest.cpp index bf7f4ae96d0..08307c10d2d 100644 --- a/unittests/Support/JSONTest.cpp +++ b/unittests/Support/JSONTest.cpp @@ -227,6 +227,66 @@ TEST(JSONTest, Inspection) { } } +// Verify special integer handling - we try to preserve exact int64 values. +TEST(JSONTest, Integers) { + struct { + const char *Desc; + Value Val; + const char *Str; + llvm::Optional AsInt; + llvm::Optional AsNumber; + } TestCases[] = { + { + "Non-integer. Stored as double, not convertible.", + double{1.5}, + "1.5", + llvm::None, + 1.5, + }, + + { + "Integer, not exact double. Stored as int64, convertible.", + int64_t{0x4000000000000001}, + "4611686018427387905", + int64_t{0x4000000000000001}, + double{0x4000000000000000}, + }, + + { + "Negative integer, not exact double. Stored as int64, convertible.", + int64_t{-0x4000000000000001}, + "-4611686018427387905", + int64_t{-0x4000000000000001}, + double{-0x4000000000000000}, + }, + + { + "Dynamically exact integer. Stored as double, convertible.", + double{0x6000000000000000}, + "6.9175290276410819e+18", + int64_t{0x6000000000000000}, + double{0x6000000000000000}, + }, + + { + "Dynamically integer, >64 bits. Stored as double, not convertible.", + 1.5 * double{0x8000000000000000}, + "1.3835058055282164e+19", + llvm::None, + 1.5 * double{0x8000000000000000}, + }, + }; + for (const auto &T : TestCases) { + EXPECT_EQ(T.Str, s(T.Val)) << T.Desc; + llvm::Expected Doc = parse(T.Str); + EXPECT_TRUE(!!Doc) << T.Desc; + EXPECT_EQ(Doc->getAsInteger(), T.AsInt) << T.Desc; + EXPECT_EQ(Doc->getAsNumber(), T.AsNumber) << T.Desc; + EXPECT_EQ(T.Val, *Doc) << T.Desc; + EXPECT_EQ(T.Str, s(*Doc)) << T.Desc; + } +} + // Sample struct with typical JSON-mapping rules. struct CustomStruct { CustomStruct() : B(false) {}