Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflo

net/base/parse_number_unittest.cc

Index: net/base/parse_number_unittest.cc
diff --git a/net/base/parse_number_unittest.cc b/net/base/parse_number_unittest.cc
index 79cac9f43ba211b26aa8d58d1c943154b387f911..878c88bb182dca44546516f093325943dc36bedf 100644
--- a/net/base/parse_number_unittest.cc
+++ b/net/base/parse_number_unittest.cc
@@ -4,67 +4,309 @@
 
 #include "net/base/parse_number.h"
 
+#include <limits>
+
+#include "base/strings/string_number_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 namespace {
 
-TEST(ParseNumberTest, IntValidInputs) {
-  const struct {
-    const char* input;
-    int output;
-  } kTests[] = {
-      {"0", 0},     {"00000", 0}, {"003", 3}, {"003", 3}, {"1234566", 1234566},
-      {"987", 987}, {"010", 10},
-  };
-
-  for (const auto& test : kTests) {
-    int result;
-    ASSERT_TRUE(ParseNonNegativeDecimalInt(test.input, &result))
-        << "Failed to parse: " << test.input;
-    EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input;
+// Increments the final character in a string by 1. Used as a simplistic way to
+// increment a numeric string (flawed, but good enough for use case).
+std::string IncrementLastChar(std::string value) {
+  value[value.size() - 1]++;
+  return value;
+}
+
+// These are valid inputs representing non-negative integers. Note that these
+// test inputs are re-used when constructing negative test cases, by simply
+// prepending a '-'.
+const struct {
+  const char* input;
+  int expected_output;
+} kValidNonNegativeTests[] = {
+    {"0", 0},     {"00000", 0}, {"003", 3}, {"003", 3}, {"1234566", 1234566},
+    {"987", 987}, {"010", 10},
+};
+
+// These are invalid inputs that can not be parsed regardless of the format
+// used (they are neither valid negative or non-negative values).
+const char* kInvalidParseTests[] = {
+    "",       "-",      "--",    "23-",  "134-34", "- ",   "    ",  "+42",
+    " 123",   "123 ",   "123\n", "0xFF", "-0xFF",  "0x11", "-0x11", "x11",
+    "-x11",   "F11",    "-F11",  "AF",   "-AF",    "0AF",  "0.0",   "13.",
+    "13,000", "13.000", "13/5",  "Inf",  "NaN",    "null", "dog",
+};
+
+// This wrapper calls func() and expects the result to match |expected_output|.
+template <typename OutputType, typename ParseFunc, typename ExpectationType>
+void ExpectParseIntSuccess(ParseFunc func,
+                           const base::StringPiece& input,
+                           ParseIntFormat format,
+                           ExpectationType expected_output) {
+  // Try parsing without specifying an error output - expecting success.
+  OutputType parsed_number1;
+  EXPECT_TRUE(func(input, format, &parsed_number1, nullptr))
+      << "Failed to parse: " << input;
+  EXPECT_EQ(static_cast<OutputType>(expected_output), parsed_number1);
+
+  // Try parsing with an error output - expecting success.
+  ParseIntError kBogusError = static_cast<ParseIntError>(19);
+  ParseIntError error = kBogusError;
+  OutputType parsed_number2;
+  EXPECT_TRUE(func(input, format, &parsed_number2, &error))
+      << "Failed to parse: " << input;
+  EXPECT_EQ(static_cast<OutputType>(expected_output), parsed_number2);
+  // Check that the error output was not written to.
+  EXPECT_EQ(kBogusError, error);
+}
+
+// This wrapper calls func() and expects the failure to match |expected_error|.
+template <typename OutputType, typename ParseFunc>
+void ExpectParseIntFailure(ParseFunc func,
+                           const base::StringPiece& input,
+                           ParseIntFormat format,
+                           ParseIntError expected_error) {
+  const OutputType kBogusOutput = OutputType(23614);

[mmenke, 2016/04/07 20:42:53]

Is the OutputType(...) needed?  Not entirely sure whether that would be
considered a C-style cast or not.

[eroman, 2016/04/08 18:57:32]

Done.

+
+  // Try parsing without specifying an error output - expecting failure.
+  OutputType parsed_number1 = kBogusOutput;
+  EXPECT_FALSE(func(input, format, &parsed_number1, nullptr))
+      << "Succeded parsing: " << input;
+  EXPECT_EQ(kBogusOutput, parsed_number1)
+      << "Modified output when failed parsing";
+
+  // Try parsing with an error output - expecting failure.
+  OutputType parsed_number2 = kBogusOutput;
+  ParseIntError error;
+  EXPECT_FALSE(func(input, format, &parsed_number2, &error))
+      << "Succeded parsing: " << input;
+  EXPECT_EQ(kBogusOutput, parsed_number2)
+      << "Modified output when failed parsing";
+  EXPECT_EQ(expected_error, error);
+}
+
+// Common tests to run for each of the versions of ParseInt*().
+template <typename T, typename ParseFunc>
+void TestParseInt(ParseFunc func) {
+  ParseIntFormat kFormats[] = {ParseIntFormat::NON_NEGATIVE,
+                               ParseIntFormat::OPTIONALLY_NEGATIVE};
+
+  // Test valid non-negative inputs
+
+  for (const auto& format : kFormats) {
+    for (const auto& test : kValidNonNegativeTests) {
+      ExpectParseIntSuccess<T>(func, test.input, format, test.expected_output);
+    }
+  }
+
+  // Test invalid inputs (invalid regardless of parsing format)
+
+  for (const auto& format : kFormats) {
+    for (const auto& input : kInvalidParseTests) {
+      ExpectParseIntFailure<T>(func, input, format,
+                               ParseIntError::FAILED_PARSE);
+    }
+  }
+
+  // Test valid negative inputs (constructed from the valid non-negative test
+  // cases).
+
+  for (const auto& format : kFormats) {
+    for (const auto& test : kValidNonNegativeTests) {
+      std::string negative_input = std::string("-") + test.input;
+      int expected_negative_output = -test.expected_output;
+
+      // The result depends on the format.
+      if (format == ParseIntFormat::NON_NEGATIVE) {
+        ExpectParseIntFailure<T>(func, negative_input, format,
+                                 ParseIntError::FAILED_PARSE);
+      } else {
+        ExpectParseIntSuccess<T>(func, negative_input, format,
+                                 expected_negative_output);
+      }
+    }
+  }
+
+  // Test parsing the largest possible value for output type.
+
+  for (const auto& format : kFormats) {
+    const T value = std::numeric_limits<T>::max();
+    ExpectParseIntSuccess<T>(func, std::to_string(value), format, value);
+  }
+
+  // Test parsing a number one larger than the output type can accomodate
+  // (overflow).
+
+  for (const auto& format : kFormats) {
+    const T value = std::numeric_limits<T>::max();
+    ExpectParseIntFailure<T>(func, IncrementLastChar(std::to_string(value)),

[mmenke, 2016/04/07 20:42:53]

Maybe worth a comment that this works because max() of all types will never end
in 9?  It's just a weird dependency (Though certainly one we can rely on), so
may be worth calling out.

[eroman, 2016/04/08 18:57:32]

Extracted to a more specific function. And added assertions rather than comments
to document this assumption.

+                             format, ParseIntError::FAILED_OVERFLOW);
+  }
+
+  // Test parsing a number at least as large as the output allows AND contains
+  // garbage at the end. This exercises an interesting internal quirk of
+  // base::StringToInt*(), in that its result cannot distinguish this case
+  // from overflow.
+
+  for (const auto& format : kFormats) {
+    const T value = std::numeric_limits<T>::max();

[mmenke, 2016/04/07 20:42:53]

Should we actually make this an overflow as well?  We default base behavior is
the same in both cases, but may be nice to use a real overflow as well.

[eroman, 2016/04/08 18:57:33]

Done.

+    ExpectParseIntFailure<T>(func, std::to_string(value) + " ", format,
+                             ParseIntError::FAILED_PARSE);
+  }
+
+  // Test parsing the smallest possible value for output type.
+
+  for (const auto& format : kFormats) {

[mmenke, 2016/04/07 20:42:53]

optional:  Rather than all these loops over formats, could just take format as
an argument, and loop over them in the fixture.  Or could make one big
loop...Fine as-is, though.

[eroman, 2016/04/08 18:57:32]

Done.
I also re-unified the tests so there isn't any duplication for the signed vs
unsigned flavors.

+    const T value = std::numeric_limits<T>::min();
+    auto str_value = std::to_string(value);

[mmenke, 2016/04/07 20:42:53]

Think it's best to just remove the auto here - think it should generally be
avoided unless the type is hideous, or it's a loop variable.

[mmenke, 2016/04/07 20:42:53]

Is std::to_string allowed C++0x11?

[eroman, 2016/04/08 18:57:32]

Done.
You are right, our C++11 guide hasn't taken a stance on this yet.
However it is already being used in the codebase...

+
+    // The minimal value is necessarily negative, since this function is testing
+    // only signed output types.
+    if (format == ParseIntFormat::NON_NEGATIVE) {
+      ExpectParseIntFailure<T>(func, str_value, format,
+                               ParseIntError::FAILED_PARSE);
+    } else {
+      ExpectParseIntSuccess<T>(func, str_value, format, value);
+    }
+  }
+
+  // Test parsing a number one less than the output type can accomodate
+  // (underflow).
+
+  {
+    std::string str_value =
+        IncrementLastChar(std::to_string(std::numeric_limits<T>::min()));

[mmenke, 2016/04/07 20:42:53]

Again, there's a weird dependency here...

[eroman, 2016/04/08 18:57:32]

No longer applicable.

+
+    ExpectParseIntFailure<T>(func, str_value,
+                             ParseIntFormat::OPTIONALLY_NEGATIVE,
+                             ParseIntError::FAILED_UNDERFLOW);
+  }
+
+  // Test parsing a string that contains a valid number followed by a NUL
+  // character.
+
+  for (const auto& format : kFormats) {
+    ExpectParseIntFailure<T>(func, base::StringPiece("123\0", 4), format,
+                             ParseIntError::FAILED_PARSE);
   }
 }
 
-TEST(ParseNumberTest, IntInvalidInputs) {
-  const char* kTests[] = {
-      "",
-      "-23",
-      "+42",
-      " 123",
-      "123 ",
-      "123\n",
-      "0xFF",
-      "0x11",
-      "x11",
-      "F11",
-      "AF",
-      "0AF",
-      "0.0",
-      "13.",
-      "13,000",
-      "13.000",
-      "13/5",
-      "9999999999999999999999999999999999999999999999999999999999999999",
-      "Inf",
-      "NaN",
-      "null",
-      "dog",
-  };
-
-  for (const auto& input : kTests) {
-    int result = 0xDEAD;
-    ASSERT_FALSE(ParseNonNegativeDecimalInt(input, &result))
-        << "Succeeded to parse: " << input;
-    EXPECT_EQ(0xDEAD, result) << "Modified output for failed parsing";
+// This wrapper calls func() and expects the result to match |expected_output|.
+template <typename OutputType, typename ParseFunc, typename ExpectationType>
+void ExpectParseUintSuccess(ParseFunc func,
+                            const base::StringPiece& input,
+                            ExpectationType expected_output) {
+  // Try parsing without specifying an error output - expecting success.
+  OutputType parsed_number1;
+  EXPECT_TRUE(func(input, &parsed_number1, nullptr)) << "Failed to parse: "
+                                                     << input;
+  EXPECT_EQ(static_cast<OutputType>(expected_output), parsed_number1);
+
+  // Try parsing with an error output - expecting success.
+  ParseIntError kBogusError = static_cast<ParseIntError>(19);
+  ParseIntError error = kBogusError;
+  OutputType parsed_number2;
+  EXPECT_TRUE(func(input, &parsed_number2, &error)) << "Failed to parse: "
+                                                    << input;
+  EXPECT_EQ(static_cast<OutputType>(expected_output), parsed_number2);
+  // Check that the error output was not written to.
+  EXPECT_EQ(kBogusError, error);
+}
+
+// This wrapper calls func() and expects the failure to match |expected_error|.
+template <typename OutputType, typename ParseFunc>
+void ExpectParseUintFailure(ParseFunc func,
+                            const base::StringPiece& input,
+                            ParseIntError expected_error) {
+  const OutputType kBogusOutput = OutputType(23614);
+
+  // Try parsing without specifying an error output - expecting failure.
+  OutputType parsed_number1 = kBogusOutput;
+  EXPECT_FALSE(func(input, &parsed_number1, nullptr)) << "Succeded parsing: "
+                                                      << input;
+  EXPECT_EQ(kBogusOutput, parsed_number1)
+      << "Modified output when failed parsing";
+
+  // Try parsing with an error output - expecting failure.
+  OutputType parsed_number2 = kBogusOutput;
+  ParseIntError error;
+  EXPECT_FALSE(func(input, &parsed_number2, &error)) << "Succeded parsing: "
+                                                     << input;
+  EXPECT_EQ(kBogusOutput, parsed_number2)
+      << "Modified output when failed parsing";
+  EXPECT_EQ(expected_error, error);
+}
+
+// Common tests to run for each of the versions of ParseUint*().
+template <typename T, typename ParseFunc>
+void TestParseUint(ParseFunc func) {
+  // Test valid non-negative inputs
+  for (const auto& test : kValidNonNegativeTests) {
+    ExpectParseUintSuccess<T>(func, test.input, test.expected_output);
+  }
+
+  // Test invalid inputs (invalid regardless of parsing format)
+  for (const auto& input : kInvalidParseTests) {
+    ExpectParseUintFailure<T>(func, input, ParseIntError::FAILED_PARSE);
   }
+
+  // Test valid negative inputs (constructed from the valid non-negative test
+  // cases).
+  for (const auto& test : kValidNonNegativeTests) {
+    std::string negative_input = std::string("-") + test.input;
+
+    // The result depends on the format.
+    ExpectParseUintFailure<T>(func, negative_input,
+                              ParseIntError::FAILED_PARSE);
+  }
+
+  // Test parsing the largest possible value for output type.
+  {
+    const T value = std::numeric_limits<T>::max();
+    ExpectParseUintSuccess<T>(func, std::to_string(value), value);
+  }
+
+  // Test parsing a number one larger than the output type can accomodate
+  // (overflow).
+  {
+    const T value = std::numeric_limits<T>::max();
+    ExpectParseUintFailure<T>(func, IncrementLastChar(std::to_string(value)),
+                              ParseIntError::FAILED_OVERFLOW);
+  }
+
+  // Test parsing a number at least as large as the output allows AND contains
+  // garbage at the end. This exercises an interesting internal quirk of
+  // base::StringToInt*(), in that its result cannot distinguish this case
+  // from overflow.
+
+  {
+    const T value = std::numeric_limits<T>::max();
+    ExpectParseUintFailure<T>(func, std::to_string(value) + " ",
+                              ParseIntError::FAILED_PARSE);
+  }
+
+  // Test parsing a string that contains a valid number followed by a NUL
+  // character.
+  ExpectParseUintFailure<T>(func, base::StringPiece("123\0", 4),
+                            ParseIntError::FAILED_PARSE);
+}
+
+TEST(ParseNumberTest, ParseInt32) {
+  TestParseInt<int32_t>(ParseInt32);
+}
+
+TEST(ParseNumberTest, ParseInt64) {
+  TestParseInt<int64_t>(ParseInt64);
+}
+
+TEST(ParseNumberTest, ParseUint32) {
+  TestParseUint<uint32_t>(ParseUint32);
 }
 
-TEST(ParseNumberTest, IntInvalidInputsContainsNul) {
-  int result = 0xDEAD;
-  ASSERT_FALSE(
-      ParseNonNegativeDecimalInt(base::StringPiece("123\0", 4), &result));
-  EXPECT_EQ(0xDEAD, result);
+TEST(ParseNumberTest, ParseUint64) {
+  TestParseUint<uint64_t>(ParseUint64);
 }
 
 }  // namespace