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

net/base/parse_number.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_PARSE_NUMBER_H_
 #define NET_BASE_PARSE_NUMBER_H_
 
 #include "base/compiler_specific.h"
 #include "base/strings/string_piece.h"
 #include "net/base/net_export.h"
 
 // This file contains utility functions for parsing numbers, in the context of
 // network protocols.
 //
 // Q: Doesn't //base already provide these in string_number_conversions.h, with
 //    functions like base::StringToInt()?
 //
 // A: Yes, and those functions are used under the hood by these
 //    implementations.
 //
 //    However using the number parsing functions from //base directly in network
 //    code can lead to subtle bugs, as the //base versions  are more permissive.
 //    For instance "+99" is successfully parsed by base::StringToInt().
 //
 //    However in the majority of places in //net, a leading plus on a number
 //    should be considered invalid. For instance when parsing a host:port pair
 //    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.
 
 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:
 //
-//  Recognized inputs take the form:
+// When sign_policy=NON_NEGATIVE it accepts inputs of 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:
+//    ("" | "-") 1*DIGIT
+//
+// Where DIGIT is an ASCII number in the range '0' - '9'
+//
+// Note that:
 //   * Parsing is locale independent
+//   * |*output| is never modified on failure
 //   * Leading zeros are allowed (numbers needn't be in minimal encoding)
-//   * Inputs that would overflow the output are rejected.
-//   * Only accepts integers
-//
-//  Examples of recognized inputs are:
-//    "13"
-//    "0"
-//    "00013"
-//
-//  Examples of rejected inputs are:
-//    "  13"
-//    "-13"
-//    "+13"
-//    "0x15"
-//    "13.3"
-NET_EXPORT bool ParseNonNegativeDecimalInt(const base::StringPiece& input,
-                                           int* output) WARN_UNUSED_RESULT;
+//   * 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
 
 #endif  // NET_BASE_PARSE_NUMBER_H_