Skip to content

Commit

Permalink
🚑 fix for #465
Browse files Browse the repository at this point in the history
  • Loading branch information
@nlohmann
nlohmann committed Feb 20, 2017
1 parent b04543e commit 7d14f16
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 22 deletions.
11 changes: 1 addition & 10 deletions src/json.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8310,16 +8310,7 @@ class basic_json
static_assert(d == 6 or d == 15 or d == 16 or d == 17,
"unexpected NumberType");

static constexpr auto fmt = d == 6 ? "%.7g"
: d == 15 ? "%.16g"
: d == 16 ? "%.17g"
: d == 17 ? "%.18g"
: "%.19g";
// I'm not sure why we need to +1 the precision,
// but without it there's a unit-test that fails
// that asserts precision of the output

snprintf(m_buf.data(), m_buf.size(), fmt, x);
snprintf(m_buf.data(), m_buf.size(), "%.*g", d, x);

// read information from locale
const auto loc = localeconv();
Expand Down
11 changes: 1 addition & 10 deletions src/json.hpp.re2c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8310,16 +8310,7 @@ class basic_json
static_assert(d == 6 or d == 15 or d == 16 or d == 17,

This comment has been minimized.

Sign in to view

Copy link
@nlohmann

nlohmann Feb 21, 2017

Author Owner

@TurpentineDistillery Which number types did you have in mind to check for 16 and 17? I only could find 6 for float and 15 for double.

This comment has been minimized.

Sign in to view

Copy link
@nlohmann

nlohmann Feb 21, 2017

Author Owner

Another question: why a buffer size of 30?

This comment has been minimized.

Sign in to view

Copy link
@TurpentineDistillery

TurpentineDistillery Feb 21, 2017

I was thinking long-double and everything inbetween. This static-assert was only necessary in the context of generating fmt string below that you got rid of. Hence this static_assert is no longer relevant.

The buffer size is 32, such that it is reasonably small, but large enough to safely accommodate largest possible string representation of a number. Choosing it to be a power of two (because why not), and accounting for leading sign and trailing zero leaves the capacity of 30.

This comment has been minimized.

Sign in to view

Copy link
@nlohmann

nlohmann Feb 21, 2017

Author Owner

Is there any upper bound to have in mind to make sure 30 is large enough?

This comment has been minimized.

Sign in to view

Copy link
@TurpentineDistillery

TurpentineDistillery Feb 21, 2017 •
edited
Loading

My thinking was, e.g. -1.23e+999 with digits10=18 would require buffer size of 26.
Now that I think about it, if snprintf was also adding some decimal grouping characters, we're getting pretty close to the cap, so maybe just bump the size to 64 instead of 32 so we don't have to think twice about it, and for posterity add an assert on the return value of snprintf .

This comment has been minimized.

Sign in to view

Copy link
@nlohmann

nlohmann Feb 21, 2017

Author Owner

That sounds reasonable. I'll take care of it.

This comment has been minimized.

Sign in to view

Copy link
@nlohmann

nlohmann Feb 21, 2017

Author Owner

I overworked the function, see 967f914#diff-ce193778faf96e9fbb27d02b2136a3ed.
"unexpected NumberType");

static constexpr auto fmt = d == 6 ? "%.7g"
: d == 15 ? "%.16g"
: d == 16 ? "%.17g"
: d == 17 ? "%.18g"
: "%.19g";
// I'm not sure why we need to +1 the precision,
// but without it there's a unit-test that fails
// that asserts precision of the output

snprintf(m_buf.data(), m_buf.size(), fmt, x);
snprintf(m_buf.data(), m_buf.size(), "%.*g", d, x);

// read information from locale
const auto loc = localeconv();
Expand Down
18 changes: 16 additions & 2 deletions test/src/unit-inspection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,17 +250,31 @@ TEST_CASE("object inspection")
ss.str(std::string());

// use stringstream for JSON serialization
json j_number = 3.141592653589793;
json j_number = 3.14159265358979;
ss << j_number;

// check that precision has been overridden during serialization
CHECK(ss.str() == "3.141592653589793");
CHECK(ss.str() == "3.14159265358979");

// check that precision has been restored
CHECK(ss.precision() == 3);
}
}

SECTION("round trips")
{
for (const auto& s :
{"3.141592653589793", "1000000000000000010E5"
})
{
json j1 = json::parse(s);
std::string s1 = j1.dump();
json j2 = json::parse(s1);
std::string s2 = j2.dump();
CHECK(s1 == s2);
}
}

SECTION("return the type of the object (explicit)")
{
SECTION("null")
Expand Down
9 changes: 9 additions & 0 deletions test/src/unit-regression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -783,4 +783,13 @@ TEST_CASE("regression tests")
json j = R"({"bool_value":true,"double_value":2.0,"int_value":10,"level1":{"list_value":[3,"hi",false],"tmp":5.0},"string_value":"hello"})"_json;
CHECK(j["double_value"].is_number_float());
}

SECTION("issue #465 - roundtrip error while parsing 1000000000000000010E5")
{
json j1 = json::parse("1000000000000000010E5");
std::string s1 = j1.dump();
json j2 = json::parse(s1);
std::string s2 = j2.dump();
CHECK(s1 == s2);
}
}

1 comment on commit 7d14f16

@TurpentineDistillery
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snprintf(m_buf.data(), m_buf.size(), "%.*g", d, x); - sorcery!

Please sign in to comment.