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.

[Ryan Sleevi, 2016/03/25 22:35:17]

s/versions  are/versions are/

[eroman, 2016/04/07 01:17:01]

Done.

 //    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.

[Ryan Sleevi, 2016/03/25 22:35:17]

s/content-length/Content-Length/

[eroman, 2016/04/07 01:17:01]

No longer applicable -I re-wrote this entire comment block to something
(hopefully) simpler.

 //
+//    Another difficulty with using base::StringToInt() is distinguishing
+//    overflow/underflow from parsing failures.

[Ryan Sleevi, 2016/03/25 22:35:17]

Not sure why this matters?

[eroman, 2016/03/25 23:02:23]

Some code wants to know when overflow has happened, and is doing it wrong today
using base::StringToInt().

At first blush one might read the documentation for base::StringToInt() and
think you can do something like this:

int64_t result;
if (!base::StringToInt(input, &result)) {
  // |result| is written to on failure. If it overflow it will be MAX_INT64
  if (result == std::numeric_limits<decltype(result)>::max()) {
      // Wee, overflow! I guess I won't fail here
  }
}

This however is buggy and wrong. I have come across several examples already.

To see how the proposed API would tackle these, here are 2 demo fixes using it:

https://codereview.chromium.org/1827243002/
https://codereview.chromium.org/1831963002/

...As I am going through and looking at the //net uses of StringToInt() I am
uncovering a number of problems. 

Ultimately these solutions may belong in //base, especially if they are
widespread outside of //net.

But for now fixing them first in //net gives good experience on what the shape
of a good API should be, and what the common errors are.

I can circle back to //base once we have solved the //net problems. And also
once I hear back from a //base owner (cc-ed thakis so far).

+//
 //    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.
+// Policy for parsing of integers that determines which form of numbers will be
+// accepted.
+enum class ParseInteger {
+  // Accept inputs of the form:
+  //
+  //    1*DIGIT
+  //
+  // This is consistent with the description in RFC 2616 for representing

[Ryan Sleevi, 2016/03/25 22:35:17]

s/2616/7230/

[eroman, 2016/04/07 01:17:01]

Done.

+  // numbers, as well as many other HTTP related standards.

[Ryan Sleevi, 2016/03/25 22:35:17]

It's not just HTTP standards (DIGIT comes from the ABNF RFC - 5234 - so it's
generally used by any textual processing)

// This construction is used in a variety of IETF standards, such
// as RFC 7230 (HTTP).

[eroman, 2016/04/07 01:17:01]

Done.

+  DISALLOW_NEGATIVE,
+
+  // Accept inputs of the form:
+  //
+  //    ["-"] 1*DIGIT
+  //
+  // In other words, the number can optional start with a negative sign. Note
+  // that -0 is also counted as a valid input.
+  ALLOW_NEGATIVE

[mmenke, 2016/03/25 22:10:56]

Think it's worth noting that if this is true, we return FAILED_PARSE on
negatives, rather than FAILED_UNDERFLOW.

[eroman, 2016/04/07 01:17:01]

You mean a comment in DISALLOW_NEGATIVE? Done.

+};
+
+// The specific reason why a ParseInteger*() function failed.
+enum class ParseIntegerError {
+  // The parsed number couldn't fit into the provided output type because it was
+  // too large.

[mmenke, 2016/03/25 22:10:56]

nit: suggest high / low instead of large/small.  Large is fine, but calling
negative numbers "small" to me seems a little unusual.

[eroman, 2016/04/07 01:17:01]

Done.

+  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 (as
+  // determined by the policy).
+  FAILED_PARSE,
+};
+
+// ParseIntegerBase10() parses a string representing a decimal number to a
+// SIGNED integer result.
 //
-//  Recognized inputs take the form:
-//    1*DIGIT
+// 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.
 //
-//  Where DIGIT is an ASCII number in the range '0' - '9'
+// On failure it is guaranteed that |*output| was not modified.
 //
-//  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
-//
-//  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;
+// See the ParseInteger enum for a description of what types of input are
+// accepted. Note that although the output type is signed, the parsing of
+// negative values can be disallowed by selecting the

[mmenke, 2016/03/25 22:10:56]

"selecting the" -> "passing" / "using"?

[eroman, 2016/04/07 01:17:01]

Done.

+// ParseInteger::DISALLOW_NEGATIVE.
+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   ParseInteger policy,
+                                   int32_t* output,
+                                   ParseIntegerError* optional_error = nullptr)

[Ryan Sleevi, 2016/03/25 22:35:17]

With respect to the defaults here, it seems like the optionality comes with a
guaranteed performance penalty (branch / branch-and-set vs an always-on set).
Why not just make the error mandatory, rather than optional?

[eroman, 2016/03/25 23:02:23]

The optionality of the error is primarily to make it simpler for the caller.
The large majority of callers don't care why it failed.

From a performance standpoint, having it optional actually saves time in the
case of parsing failures. Since extra checks need to be done to differentiate
parsing failure from underflow/overflow in the implementation which are skipped
when it is nullptr.

At any rate, that extra branch is only in the case of a failure, so I think ease
of use is the more important metric here.

+    WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseIntegerBase10(const base::StringPiece& input,
+                                   ParseInteger policy,
+                                   int64_t* output,
+                                   ParseIntegerError* optional_error = nullptr)
+    WARN_UNUSED_RESULT;

[Ryan Sleevi, 2016/03/25 22:35:17]

I would argue that overloading like this violates the style guide's admonitions
in https://google.github.io/styleguide/cppguide.html#Function_Overloading

//base does good at this (StringToInt / StringToInt64 / StringToSizeT).

This would also address the "Base10" suffix vs //base's approach (StringTo... vs
HexStringTo...)

[mmenke, 2016/03/25 22:49:59]

"Use overloaded functions (including constructors) only if a reader looking at a
call site can get a good idea of what is happening without having to first
figure out exactly which overload is being called."

I think a reader looking at callsites doesn't really need to know or care which
overload is being called, so seems to fit the policy.  I'm happy, either way,
just trying to understand your thinking here.

[eroman, 2016/03/25 23:02:23]

For technical reasons I am going to have to avoid overloading because of size_t
being a special case.

In general, my preference for overloading is because it simplifies use in
generic code.

For example, once I change this to unique names the unit-test code will need
more work to share a common test driver, whereas right now a single template
paramertized on type and done.

+
+// ParseUnsignedIntegerBase10() is the same as calling ParseIntegerBase10()
+// except:
+//  * The output type is unsigned
+//  * The policy is implied to be ParseInteger::DISALLOW_NEGATIVE
+NET_EXPORT bool ParseUnsignedIntegerBase10(
+    const base::StringPiece& input,
+    uint32_t* output,
+    ParseIntegerError* optional_error = nullptr) WARN_UNUSED_RESULT;
+
+NET_EXPORT bool ParseUnsignedIntegerBase10(
+    const base::StringPiece& input,
+    uint64_t* output,
+    ParseIntegerError* optional_error = nullptr) WARN_UNUSED_RESULT;
 
 }  // namespace net
 
 #endif  // NET_BASE_PARSE_NUMBER_H_