Number fixes (#1053)
* cleaning up the logic for parsing numbers
* Add Testcases for new Reader in jsontestrunner
diff --git a/src/jsontestrunner/main.cpp b/src/jsontestrunner/main.cpp
index cb9db66..dbfbe98 100644
--- a/src/jsontestrunner/main.cpp
+++ b/src/jsontestrunner/main.cpp
@@ -15,7 +15,9 @@
#include <algorithm> // sort
#include <cstdio>
+#include <iostream>
#include <json/json.h>
+#include <memory>
#include <sstream>
struct Options {
@@ -126,19 +128,45 @@
const Json::String& actual,
const Json::String& kind,
const Json::Features& features, bool parseOnly,
- Json::Value* root) {
- Json::Reader reader(features);
- bool parsingSuccessful =
- reader.parse(input.data(), input.data() + input.size(), *root);
- if (!parsingSuccessful) {
- printf("Failed to parse %s file: \n%s\n", kind.c_str(),
- reader.getFormattedErrorMessages().c_str());
- return 1;
+ Json::Value* root, bool use_legacy) {
+ if (!use_legacy) {
+ Json::CharReaderBuilder builder;
+
+ builder.settings_["allowComments"] = features.allowComments_;
+ builder.settings_["strictRoot"] = features.strictRoot_;
+ builder.settings_["allowDroppedNullPlaceholders"] =
+ features.allowDroppedNullPlaceholders_;
+ builder.settings_["allowNumericKeys"] = features.allowNumericKeys_;
+
+ std::unique_ptr<Json::CharReader> reader(builder.newCharReader());
+ Json::String errors;
+ const bool parsingSuccessful =
+ reader->parse(input.data(), input.data() + input.size(), root, &errors);
+
+ if (!parsingSuccessful) {
+ std::cerr << "Failed to parse " << kind << " file: " << std::endl
+ << errors << std::endl;
+ return 1;
+ }
+
+ // We may instead check the legacy implementation (to ensure it doesn't
+ // randomly get broken).
+ } else {
+ Json::Reader reader(features);
+ const bool parsingSuccessful =
+ reader.parse(input.data(), input.data() + input.size(), *root);
+ if (!parsingSuccessful) {
+ std::cerr << "Failed to parse " << kind << " file: " << std::endl
+ << reader.getFormatedErrorMessages() << std::endl;
+ return 1;
+ }
}
+
if (!parseOnly) {
FILE* factual = fopen(actual.c_str(), "wt");
if (!factual) {
- printf("Failed to create %s actual file.\n", kind.c_str());
+ std::cerr << "Failed to create '" << kind << "' actual file."
+ << std::endl;
return 2;
}
printValueTree(factual, *root);
@@ -172,7 +200,7 @@
*rewrite = write(root);
FILE* fout = fopen(rewritePath.c_str(), "wt");
if (!fout) {
- printf("Failed to create rewrite file: %s\n", rewritePath.c_str());
+ std::cerr << "Failed to create rewrite file: " << rewritePath << std::endl;
return 2;
}
fprintf(fout, "%s\n", rewrite->c_str());
@@ -193,14 +221,15 @@
static void printConfig() {
// Print the configuration used to compile JsonCpp
#if defined(JSON_NO_INT64)
- printf("JSON_NO_INT64=1\n");
+ std::cout << "JSON_NO_INT64=1" << std::endl;
#else
- printf("JSON_NO_INT64=0\n");
+ std::cout << "JSON_NO_INT64=0" << std::endl;
#endif
}
static int printUsage(const char* argv[]) {
- printf("Usage: %s [--strict] input-json-file", argv[0]);
+ std::cout << "Usage: " << argv[0] << " [--strict] input-json-file"
+ << std::endl;
return 3;
}
@@ -230,7 +259,7 @@
} else if (writerName == "BuiltStyledStreamWriter") {
opts->write = &useBuiltStyledStreamWriter;
} else {
- printf("Unknown '--json-writer %s'\n", writerName.c_str());
+ std::cerr << "Unknown '--json-writer' " << writerName << std::endl;
return 4;
}
}
@@ -240,19 +269,20 @@
opts->path = argv[index];
return 0;
}
-static int runTest(Options const& opts) {
+
+static int runTest(Options const& opts, bool use_legacy) {
int exitCode = 0;
Json::String input = readInputTestFile(opts.path.c_str());
if (input.empty()) {
- printf("Failed to read input or empty input: %s\n", opts.path.c_str());
+ std::cerr << "Invalid input file: " << opts.path << std::endl;
return 3;
}
Json::String basePath = removeSuffix(opts.path, ".json");
if (!opts.parseOnly && basePath.empty()) {
- printf("Bad input path. Path does not end with '.expected':\n%s\n",
- opts.path.c_str());
+ std::cerr << "Bad input path '" << opts.path
+ << "'. Must end with '.expected'" << std::endl;
return 3;
}
@@ -262,34 +292,47 @@
Json::Value root;
exitCode = parseAndSaveValueTree(input, actualPath, "input", opts.features,
- opts.parseOnly, &root);
+ opts.parseOnly, &root, use_legacy);
if (exitCode || opts.parseOnly) {
return exitCode;
}
+
Json::String rewrite;
exitCode = rewriteValueTree(rewritePath, root, opts.write, &rewrite);
if (exitCode) {
return exitCode;
}
+
Json::Value rewriteRoot;
exitCode = parseAndSaveValueTree(rewrite, rewriteActualPath, "rewrite",
- opts.features, opts.parseOnly, &rewriteRoot);
- if (exitCode) {
- return exitCode;
- }
- return 0;
+ opts.features, opts.parseOnly, &rewriteRoot,
+ use_legacy);
+
+ return exitCode;
}
+
int main(int argc, const char* argv[]) {
Options opts;
try {
int exitCode = parseCommandLine(argc, argv, &opts);
if (exitCode != 0) {
- printf("Failed to parse command-line.");
+ std::cerr << "Failed to parse command-line." << std::endl;
return exitCode;
}
- return runTest(opts);
+
+ const int modern_return_code = runTest(opts, false);
+ if (modern_return_code) {
+ return modern_return_code;
+ }
+
+ const std::string filename =
+ opts.path.substr(opts.path.find_last_of("\\/") + 1);
+ const bool should_run_legacy = (filename.rfind("legacy_", 0) == 0);
+ if (should_run_legacy) {
+ return runTest(opts, true);
+ }
} catch (const std::exception& e) {
- printf("Unhandled exception:\n%s\n", e.what());
+ std::cerr << "Unhandled exception:" << std::endl << e.what() << std::endl;
return 1;
}
}
diff --git a/src/lib_json/json_reader.cpp b/src/lib_json/json_reader.cpp
index 38aab78..0c1e88d 100644
--- a/src/lib_json/json_reader.cpp
+++ b/src/lib_json/json_reader.cpp
@@ -1540,19 +1540,45 @@
// larger than the maximum supported value of an integer then
// we decode the number as a double.
Location current = token.start_;
- bool isNegative = *current == '-';
- if (isNegative)
+ const bool isNegative = *current == '-';
+ if (isNegative) {
++current;
+ }
- static constexpr auto positive_threshold = Value::maxLargestUInt / 10;
- static constexpr auto positive_last_digit = Value::maxLargestUInt % 10;
- static constexpr auto negative_threshold =
- Value::LargestUInt(Value::minLargestInt) / 10;
- static constexpr auto negative_last_digit =
- Value::LargestUInt(Value::minLargestInt) % 10;
+ // We assume we can represent the largest and smallest integer types as
+ // unsigned integers with separate sign. This is only true if they can fit
+ // into an unsigned integer.
+ static_assert(Value::maxLargestInt <= Value::maxLargestUInt,
+ "Int must be smaller than UInt");
- const auto threshold = isNegative ? negative_threshold : positive_threshold;
- const auto last_digit =
+ // We need to convert minLargestInt into a positive number. The easiest way
+ // to do this conversion is to assume our "threshold" value of minLargestInt
+ // divided by 10 can fit in maxLargestInt when absolute valued. This should
+ // be a safe assumption.
+ static_assert(Value::minLargestInt <= -Value::maxLargestInt,
+ "The absolute value of minLargestInt must be greater than or "
+ "equal to maxLargestInt");
+ static_assert(Value::minLargestInt / 10 >= -Value::maxLargestInt,
+ "The absolute value of minLargestInt must be only 1 magnitude "
+ "larger than maxLargest Int");
+
+ static constexpr Value::LargestUInt positive_threshold =
+ Value::maxLargestUInt / 10;
+ static constexpr Value::UInt positive_last_digit = Value::maxLargestUInt % 10;
+
+ // For the negative values, we have to be more careful. Since typically
+ // -Value::minLargestInt will cause an overflow, we first divide by 10 and
+ // then take the inverse. This assumes that minLargestInt is only a single
+ // power of 10 different in magnitude, which we check above. For the last
+ // digit, we take the modulus before negating for the same reason.
+ static constexpr Value::LargestUInt negative_threshold =
+ Value::LargestUInt(-(Value::minLargestInt / 10));
+ static constexpr Value::UInt negative_last_digit =
+ Value::UInt(-(Value::minLargestInt % 10));
+
+ const Value::LargestUInt threshold =
+ isNegative ? negative_threshold : positive_threshold;
+ const Value::UInt max_last_digit =
isNegative ? negative_last_digit : positive_last_digit;
Value::LargestUInt value = 0;
@@ -1561,26 +1587,30 @@
if (c < '0' || c > '9')
return decodeDouble(token, decoded);
- const auto digit(static_cast<Value::UInt>(c - '0'));
+ const Value::UInt digit(static_cast<Value::UInt>(c - '0'));
if (value >= threshold) {
// We've hit or exceeded the max value divided by 10 (rounded down). If
// a) we've only just touched the limit, meaing value == threshold,
// b) this is the last digit, or
// c) it's small enough to fit in that rounding delta, we're okay.
// Otherwise treat this number as a double to avoid overflow.
- if (value > threshold || current != token.end_ || digit > last_digit) {
+ if (value > threshold || current != token.end_ ||
+ digit > max_last_digit) {
return decodeDouble(token, decoded);
}
}
value = value * 10 + digit;
}
- if (isNegative)
- decoded = -Value::LargestInt(value);
- else if (value <= Value::LargestUInt(Value::maxLargestInt))
+ if (isNegative) {
+ // We use the same magnitude assumption here, just in case.
+ const Value::UInt last_digit = static_cast<Value::UInt>(value % 10);
+ decoded = -Value::LargestInt(value / 10) * 10 - last_digit;
+ } else if (value <= Value::LargestUInt(Value::maxLargestInt)) {
decoded = Value::LargestInt(value);
- else
+ } else {
decoded = value;
+ }
return true;
}
@@ -1597,37 +1627,12 @@
bool OurReader::decodeDouble(Token& token, Value& decoded) {
double value = 0;
- const int bufferSize = 32;
- int count;
- ptrdiff_t const length = token.end_ - token.start_;
-
- // Sanity check to avoid buffer overflow exploits.
- if (length < 0) {
- return addError("Unable to parse token length", token);
- }
- auto const ulength = static_cast<size_t>(length);
-
- // Avoid using a string constant for the format control string given to
- // sscanf, as this can cause hard to debug crashes on OS X. See here for more
- // info:
- //
- // http://developer.apple.com/library/mac/#DOCUMENTATION/DeveloperTools/gcc-4.0.1/gcc/Incompatibilities.html
- char format[] = "%lf";
-
- if (length <= bufferSize) {
- Char buffer[bufferSize + 1];
- memcpy(buffer, token.start_, ulength);
- buffer[length] = 0;
- fixNumericLocaleInput(buffer, buffer + length);
- count = sscanf(buffer, format, &value);
- } else {
- String buffer(token.start_, token.end_);
- count = sscanf(buffer.c_str(), format, &value);
- }
-
- if (count != 1)
+ const String buffer(token.start_, token.end_);
+ IStringStream is(buffer);
+ if (!(is >> value)) {
return addError(
"'" + String(token.start_, token.end_) + "' is not a number.", token);
+ }
decoded = value;
return true;
}
@@ -1649,9 +1654,9 @@
Location end = token.end_ - 1; // do not include '"'
while (current != end) {
Char c = *current++;
- if (c == '"')
+ if (c == '"') {
break;
- else if (c == '\\') {
+ } else if (c == '\\') {
if (current == end)
return addError("Empty escape sequence in string", token, current);
Char escape = *current++;