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..871c72c360c3ab64469ce6068a0b08a4c7323b1f 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,90 @@ 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.
+// Possible return values from ParseInteger().
+enum class ParseIntegerResult {
+  // Success!
+  OK,
+
+  // 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. Since in this domain a leading '-' is
+  // never accepted, even if the end result was non-negative (like "-0").
+  FAILED_PARSE,

[mmenke, 2016/03/24 21:41:48]

I'm a bit wary of these multiple-failure-value functions.  The fact that "if
(ParseNonNegativeInteger(...))" works, but "if (ParseInteger(...))" does not
seems very unexpected to me.  Enum classes prevent both incorrect cases from
compiling (Using ParseInteger() as a bool, or comparing
ParseNonNegativeInteger() with a ParseIntegerResult), but I wonder if we can do
something better.

I'll think a bit about it, and see if I can suggest something better tomorrow,
but thought I'd bring it up now, to see if you have any thoughts on it.

[eroman, 2016/03/24 22:12:16]

That is a valid concern.

One idea would be to unify them into a single function like:

bool ParseDecimalInteger(const base::StringPiece& input,
                                            bool allow_negatives,
                                            T* out,
                                            ParseDecimalIntegerError*
optional_error) WARN_UNUSED_RESULT;

 * The error output parameter could be nullptr when callers don't care about the
cause of the error.
 * No longer have special name for "ParseNonNegativeInteger"
 * Use a boolean rather than enum class for policy, to make it less cumbersome.
Or could have a different name as before.

WDYT?

Let me give it a try and see how it feels..

+};
+
+// The policy regarding negative numbers.
+enum class ParseIntegerSignPolicy {
+  // Reject any number is negative. This includes "-0". Another way to think
+  // about this policy is that it matches only 1*DIGIT.
+  NON_NEGATIVE,
+
+  // Accept both positive and negative numbers, so long as they fit into the
+  // output type.
+  //
+  // TODO(eroman): There is however a quirk in that "-0" is acepted when the
+  // output type is signed, but is rejected when the output type is unsigned
+  // (failing with an underflow).
+  ANY,
+};
+
+// ParseInteger() parses a string representing a decimal number.
+// Returns true on success, and fills |*output| with the result:
+//
+// When sign_policy=NON_NEGATIVE it accepts inputs of the form:
 //
-//  Recognized inputs take the form:
 //    1*DIGIT
 //
-//  Where DIGIT is an ASCII number in the range '0' - '9'
+// Whereas when sign_policy=ANY it accepts inputs of the form:
 //
-//  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"
+// Where DIGIT is an ASCII number in the 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;
+// Note that:
+//   * Parsing is locale independent
+//   * |*output| is never modified on failure
+//   * Leading zeros are allowed (numbers needn't be in minimal encoding)
+//   * Zero can be expressed as a negative or positive quantity.
+//     TODO(eroman): However -0 is rejected when the output type is unsigned.
+NET_EXPORT ParseIntegerResult ParseInteger(const base::StringPiece& input,
+                                           ParseIntegerSignPolicy sign_policy,
+                                           int32_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT ParseIntegerResult ParseInteger(const base::StringPiece& input,
+                                           ParseIntegerSignPolicy sign_policy,
+                                           uint32_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT ParseIntegerResult ParseInteger(const base::StringPiece& input,
+                                           ParseIntegerSignPolicy sign_policy,
+                                           int64_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT ParseIntegerResult ParseInteger(const base::StringPiece& input,
+                                           ParseIntegerSignPolicy sign_policy,
+                                           uint64_t* output) WARN_UNUSED_RESULT;
+
+// Shorter aliases for ParseInteger() where sign_policy=NON_NEGATIVE, and the
+// return value is true only when the result was OK and |*output| was written
+// to.
+NET_EXPORT bool ParseNonNegativeInteger(const base::StringPiece& input,
+                                        int32_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseNonNegativeInteger(const base::StringPiece& input,
+                                        uint32_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseNonNegativeInteger(const base::StringPiece& input,
+                                        int64_t* output) WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseNonNegativeInteger(const base::StringPiece& input,
+                                        uint64_t* output) WARN_UNUSED_RESULT;
 
 }  // namespace net