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..d5ac6d0eaada432efff96ecf7ffb9b2ddcf97eed 100644
--- a/net/base/parse_number_unittest.cc
+++ b/net/base/parse_number_unittest.cc
@@ -4,67 +4,236 @@
 
 #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) {
+// 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;
+}
+
+// Common tests to run for each of the overloaded versions of
+// ParseIntegerBase10(). Most checks are the same, however there
+// are differences based on the signedness of the output type T,
+// and its range.
+template <typename T>
+void TestParseIntegerBase10() {
+  bool kAllowNegativeChoices[] = {true /*Allow negatives*/,
+                                  false /*Don't allow negatives*/};
+
+  // Arbitrary constant, used to test if failure modifies the output parameter.
+  const T kFailResult = T(23614);
+
+  // ------------------------------------
+  // Valid (non-negative) inputs
+  // ------------------------------------
   const struct {
     const char* input;
-    int output;
-  } kTests[] = {
+    T output;

[mmenke, 2016/03/25 15:31:50]

output -> expected_output (For all of these)?

[eroman, 2016/03/25 19:04:30]

Done.

+  } kValidTests[] = {
       {"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;
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    for (const auto& test : kValidTests) {
+      T result;
+
+      EXPECT_TRUE(
+          ParseIntegerBase10(test.input, allow_negative, &result, nullptr))
+          << "Failed to parse: " << test.input;

[mmenke, 2016/03/25 15:31:49]

I'm paranoid.  Call it with a non-NULL 4th argument as well?  Want to be sure we
have a non-nullptr last argument in success case as well as a nullptr one.  We
do have some below, but think having it here provides a better guarantee.

[eroman, 2016/03/25 19:04:30]

Good idea, done.

In fact having those helpers simplifies duplication in the tests and makes it
more readable.

Consistently verifyin that:
  * error can always be optionally nullptr (for both success and failures)
  * error is not written to on success
  * output is not written to on failure

+
+      EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input;

[mmenke, 2016/03/25 15:31:50]

Expected should be first

[eroman, 2016/03/25 19:04:30]

Done.

+    }
   }
-}
 
-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",
+  // ------------------------------------
+  // Invalid inputs (regardless of sign policy)
+  // ------------------------------------
+
+  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",
   };
 
-  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";
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    for (const auto& input : kInvalidParseTests) {
+      T result = kFailResult;
+
+      ParseIntegerError error;
+      EXPECT_FALSE(ParseIntegerBase10(input, allow_negative, &result, &error));

[mmenke, 2016/03/25 15:31:50]

Similar to comment above, we should have failure tests where the 4th parameter
is nullptr.  May even want to make a template method that calls it with both,
and compares the result, and just use that everywhere.

[eroman, 2016/03/25 19:04:30]

Done (the latter)

+      EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error)
+          << "Unexpected error parsing: " << input;
+
+      // On failure the output should not have been changed.
+      EXPECT_EQ(kFailResult, result) << "Modified output for failed parsing";
+    }
+  }
+
+  // ------------------------------------
+  // Valid negative inputs.
+  // ------------------------------------
+
+  const struct {
+    const char* input;
+    int output;  // Using |int| rather than T because T may be unsigned.
+  } kValidNegativeTests[] = {
+      {"-0", 0}, {"-0001", -1}, {"-134", -134}, {"-4135", -4135},
+  };
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    for (const auto& test : kValidNegativeTests) {
+      T result = kFailResult;
+      ParseIntegerError error;
+      bool success =
+          ParseIntegerBase10(test.input, allow_negative, &result, &error);
+
+      // The result depends on the sign policy, and the signedness of the
+      // output type.
+      if (allow_negative == false) {
+        // If non-negative numbers were requested, should result in parse
+        // failure (even for "-0" which is in some sense non-negative).
+        EXPECT_FALSE(success);
+        EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+        EXPECT_EQ(kFailResult, result);
+      } else if (!std::numeric_limits<T>::is_signed) {
+        // If the output type was unsigned but the the input was negative,
+        // can't have succeeded.
+        // TODO(eroman): This applies even to "-0" even though it technically
+        // could have been parsed without "underflow". This is a consequence of
+        // how base::StringToIntXXX() works for unsigned types. Meh.
+        EXPECT_FALSE(success);
+        EXPECT_EQ(ParseIntegerError::FAILED_UNDERFLOW, error);
+        EXPECT_EQ(kFailResult, result);
+      } else {
+        // Otherwise success.
+        EXPECT_TRUE(success);
+        EXPECT_EQ(test.output, static_cast<int>(result));
+      }
+    }
+  }
+
+  // ------------------------------------
+  // Parse maximal value
+  // ------------------------------------
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    const T value = std::numeric_limits<T>::max();
+    T result;
+    EXPECT_TRUE(ParseIntegerBase10(std::to_string(value), allow_negative,
+                                   &result, nullptr));
+    EXPECT_EQ(value, result);
+  }
+
+  // ------------------------------------
+  // Parse one beyond maximal value (overflow)
+  // ------------------------------------
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(IncrementLastChar(std::to_string(value)),
+                                    allow_negative, &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_OVERFLOW, error);
+    EXPECT_EQ(kFailResult, result);
+  }
+
+  // ------------------------------------
+  // Parse maximal value with trailing whitespace
+  // ------------------------------------
+
+  // base::StringToIntXXX() has weird quirks dealing with whitespace. This
+  // exercises an incorrect way to detect overflow from base::StringToInt().
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    const T value = std::numeric_limits<T>::max();
+    T result = kFailResult;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(std::to_string(value) + " ", allow_negative,
+                                    &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+    EXPECT_EQ(kFailResult, result);
   }
+
+  // ------------------------------------
+  // Parse minimal value
+  // ------------------------------------
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    const T value = std::numeric_limits<T>::min();
+    T result = kFailResult;
+    ParseIntegerError error;
+    auto success = ParseIntegerBase10(std::to_string(value), allow_negative,
+                                      &result, &error);
+
+    if (std::numeric_limits<T>::is_signed && !allow_negative) {
+      EXPECT_FALSE(success);
+      EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+      EXPECT_EQ(kFailResult, result);
+    } else {
+      EXPECT_TRUE(success);
+      EXPECT_EQ(value, result);

[mmenke, 2016/03/25 15:31:49]

Tests in this case just seems weird...I'd skip this test for unsigned types. 
Could do -1 in that case, but you already do (basically) that higher up.

[eroman, 2016/03/25 19:04:30]

Done.

While I was at it, I also removed duplication between valid non-negative and
valid negative tests. There is just one list of valid non-negative tests, and
the valid negative tests are constructed from that by prefixing a '-'.

+    }
+  }
+
+  // ------------------------------------
+  // Parse one less than minimal value
+  // ------------------------------------
+  {
+    std::string value_str;
+
+    if (std::numeric_limits<T>::is_signed) {
+      value_str =
+          IncrementLastChar(std::to_string(std::numeric_limits<T>::min()));
+    } else {
+      value_str = "-1";
+    }
+
+    T result = kFailResult;
+    ParseIntegerError error;
+    EXPECT_FALSE(ParseIntegerBase10(value_str, true, &result, &error));
+    EXPECT_EQ(ParseIntegerError::FAILED_UNDERFLOW, error);
+    EXPECT_EQ(kFailResult, result);
+  }
+
+  // ------------------------------------
+  // Parse a value containing embedded NUL
+  // ------------------------------------
+
+  for (const auto& allow_negative : kAllowNegativeChoices) {
+    T result = kFailResult;
+    ParseIntegerError error;
+    ASSERT_FALSE(ParseIntegerBase10(base::StringPiece("123\0", 4),
+                                    allow_negative, &result, &error));
+    EXPECT_EQ(kFailResult, result);
+    EXPECT_EQ(ParseIntegerError::FAILED_PARSE, error);
+  }
+}
+
+TEST(ParseNumberTest, ParseIntegerBase10Int32) {

[mmenke, 2016/03/25 15:31:49]

I guess TEST_P can't handle templates?

[eroman, 2016/03/25 19:04:29]

I couldn't figure out how to make TEST_P handle this, in a manner that was
cleaner than just calling explicitly. Open to ideas though.

Also as an aside, I tried to strike a balance between readability and
code-duplication.

The Parse{Signed}IntegerBase10 share many tests, but differ on some others as
well as expectations.

Tying to make one mega-function for it was a little bit too spaghetti for my
tastes, so I opted for some duplication instead.

+  TestParseIntegerBase10<int32_t>();
+}
+
+TEST(ParseNumberTest, ParseIntegerBase10Uint32) {
+  TestParseIntegerBase10<uint32_t>();
+}
+
+TEST(ParseNumberTest, ParseIntegerBase10Int64) {
+  TestParseIntegerBase10<int64_t>();
 }
 
-TEST(ParseNumberTest, IntInvalidInputsContainsNul) {
-  int result = 0xDEAD;
-  ASSERT_FALSE(
-      ParseNonNegativeDecimalInt(base::StringPiece("123\0", 4), &result));
-  EXPECT_EQ(0xDEAD, result);
+TEST(ParseNumberTest, ParseIntegerBase10Uint64) {
+  TestParseIntegerBase10<uint64_t>();
 }
 
 }  // namespace