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

net/base/parse_number.h

Index: net/base/parse_number.h
diff --git a/net/base/parse_number.h b/net/base/parse_number.h
index 0b88cf0435b3af69d1330c33d07acdbc11c60e52..516550c29dfe79b47e23404065a75b8f5903152f 100644
--- a/net/base/parse_number.h
+++ b/net/base/parse_number.h
@@ -27,6 +27,9 @@
 //    you wouldn't want to recognize "foo:+99" as having a port of 99. The same
 //    issue applies when parsing a content-length header.
 //
+//    Another difficulty with using base::StringToInt() is distinguishing
+//    overflow/underflow from parsing failures.
+//
 //    To reduce the risk of such problems, use of these functions over the
 //    //base versions.
 
@@ -34,34 +37,74 @@ class GURL;
 
 namespace net {
 
-//  Parses a string representing a decimal number to an |int|. Returns true on
-//  success, and fills |*output| with the result. Note that  |*output| is not
-//  modified on failure.
+// The specific reason why ParseIntegerBase10 failed.
+enum class ParseIntegerError {
+  // The parsed number couldn't fit into the provided output type because it was
+  // too large.
+  FAILED_OVERFLOW,
+
+  // The parsed number couldn't fit into the provided output type because it was
+  // too small (negative).
+  FAILED_UNDERFLOW,
+
+  // The number failed to be parsed because it wasn't a valid decimal number.
+  // This includes the case where the number was negative but the sign-policy
+  // requested a non-negative number.
+  FAILED_PARSE,
+};
+
+// ParseIntegerBase10() parses a string representing a decimal number.
+// Returns true on success, and fills |*output| with the result. If
+// |optional_error| was non-null, then it is filled with the reason for the
+// failure.
+//
+// When allow_negative==true, accepted inputs are of the form:
 //
-//  Recognized inputs take the form:
 //    1*DIGIT
 //
-//  Where DIGIT is an ASCII number in the range '0' - '9'
+// Whereas when allow_negative==false, accepted inputs may be optionally

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

I think you've switched the true and false cases.

[eroman, 2016/03/25 17:08:09]

Oops, indeed

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

Done - this is no longer applicable.

+// preceded with a negative:
 //
-//  Note that:
-//   * Parsing is locale independent
-//   * Leading zeros are allowed (numbers needn't be in minimal encoding)
-//   * Inputs that would overflow the output are rejected.
-//   * Only accepts integers
+//    ("" | "-") 1*DIGIT
 //
-//  Examples of recognized inputs are:
-//    "13"
-//    "0"
-//    "00013"
+// DIGIT is a base-10 digit in the ASCII range '0' - '9'
 //
-//  Examples of rejected inputs are:
-//    "  13"
-//    "-13"
-//    "+13"
-//    "0x15"
-//    "13.3"
-NET_EXPORT bool ParseNonNegativeDecimalInt(const base::StringPiece& input,
-                                           int* output) WARN_UNUSED_RESULT;
+// (By our definition, "-0" is only allowed when allow_negative==false)
+//
+// This function is appropriate for use in places that parse HTTP header
+// style numbers which are defined as 1*DIGIT.
+//
+// Note that:
+//   * Parsing is locale independent
+//   * |*output| is NEVER modified on failure
+//   * |optional_error| can be nullptr
+//   * Leading zeros ARE allowed on any number
+//   * Zero can be expressed as either a positive or negative quantity,
+//   TODO(eroman): however "-0" is rejected when the output type is unsigned
+//   due to an implementation reason

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

Is this really a TODO?  If we don't accept negative numbers in one place, I
don't think we really want to accept -0, either.

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

Done: No longer applicable, due to signed/unsigned split.

+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   bool allow_negative,
+                                   int32_t* output,
+                                   ParseIntegerError* optional_error)
+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   bool allow_negative,
+                                   uint32_t* output,
+                                   ParseIntegerError* optional_error)
+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   bool allow_negative,
+                                   int64_t* output,
+                                   ParseIntegerError* optional_error)
+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   bool allow_negative,

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

Suggest making this an enum - makes for clearer, if more wordy, callsites.

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

Done.

I named the enum "ParseInteger" to make the callers look nice. But conceptually
it is ParseInteger[Policy]

+                                   uint64_t* output,

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

allow_negative + uint seems weird.  I'd suggest removing the argument (And maybe
renaming the methods?) for unsigned ints.  For your tests, could just make
wrappers.  Think cleaner production code outweighs cleaner tests.

[eroman, 2016/03/25 17:08:09]

Agreed, that was weird, and also agreed it looks cleaner with an explicitly
named enum over bool.

OK, I have updated the interface in this latest patchset to address that, as
well as changing some consumes to see how the API feels. WDYT?

I also opted to use a default parameter for the nullable errors, since that made
the code more readable. I realize this isn't commonly used in Chromium code, but
it is now allowed by the Google C++ guide so I think I can justify it.

[mmenke, 2016/03/25 17:36:32]

I'm fine with the default argument values.  I almost suggested them myself,
despite having a general preference that we avoid them.

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

Ack

+                                   ParseIntegerError* optional_error)
+    WARN_UNUSED_RESULT;
 
 }  // namespace net