Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828103002:
Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflo (Closed)
DescriptionExtend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow.
BUG=596523
Committed: https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa
Cr-Commit-Position: refs/heads/master@{#386425}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase onto origin/master #Patch Set 3 : Add more generic version (used for saturating conversions) #Patch Set 4 : rebase #Patch Set 5 : remove http_response_headers changes (moving to other CL) #
Total comments: 2
Patch Set 6 : Change API to use 1 unified function called ParseIntegerBase10() #Patch Set 7 : improve comments #
Total comments: 25
Patch Set 8 : update interface only (didn't address other comments yet) #
Total comments: 2
Patch Set 9 : Address remaining feedback #
Total comments: 22
Patch Set 10 : rebase #Patch Set 11 : address comments #Patch Set 12 : remove commentary on size_t... will shave that yak another time #
Total comments: 21
Patch Set 13 : refactor unittests #Patch Set 14 : more comment re-works #
Total comments: 4
Patch Set 15 : comment fix #
Total comments: 4
Patch Set 16 : fix comments #
Dependent Patchsets: Messages
Total messages: 42 (13 generated)
Description was changed from
==========
Extend net/base/parse_numbers.h for parsing signed numbers.
* Added ParseSignedInteger(), which parses decimal numbers (without leading +).
* Renamed ParseNonNegativeDecimalInt() to ParseNonNegativeInteger()
* Added overloads for {32bit, 64bit} x {signed, unsigned} where applicable
BUG=596523
==========
to
==========
Extend net/base/parse_number.h for parsing signed numbers.
* Added ParseSignedInteger(), which parses decimal numbers (without leading +).
* Renamed ParseNonNegativeDecimalInt() to ParseNonNegativeInteger()
* Added overloads for {32bit, 64bit} x {signed, unsigned} where applicable
BUG=596523
==========
Description was changed from
==========
Extend net/base/parse_number.h for parsing signed numbers.
* Added ParseSignedInteger(), which parses decimal numbers (without leading +).
* Renamed ParseNonNegativeDecimalInt() to ParseNonNegativeInteger()
* Added overloads for {32bit, 64bit} x {signed, unsigned} where applicable
BUG=596523
==========
to
==========
Extend net/base/parse_number.h for parsing of signed numbers.
There are now two classes of functions:
* ParseNonNegativeInteger() -- parses a decimal number (rejects if negative).
Has overloads for int32_t, uint32_t, int64_t, uint64_t.
* ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t
and int64_t. The name includes "Signed" to emphasize that this includes negative
numbers.
BUG=596523
==========
Description was changed from ========== Extend net/base/parse_number.h for parsing of signed numbers. There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for int32_t, uint32_t, int64_t, uint64_t. * ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t and int64_t. The name includes "Signed" to emphasize that this includes negative numbers. BUG=596523 ========== to ========== Extend net/base/parse_number.h for parsing of signed numbers. There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for int32_t, uint32_t, int64_t, uint64_t. This is the most commonly used function, especially in conjunction with int32_t. * ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t and int64_t. The name includes "Signed" to emphasize that this includes negative numbers. This is not used yet, however will be used soon by some follow-up CLs (certainly the 64-bit version is needed, may not need the 32-bit one though). BUG=596523 ==========
eroman@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1828103002/diff/1/net/base/parse_number_unitt... File net/base/parse_number_unittest.cc (right): https://codereview.chromium.org/1828103002/diff/1/net/base/parse_number_unitt... net/base/parse_number_unittest.cc:36: "", "-23", "+42", " 123", "123 ", "123\n", "0xFF", "0x11", "x11", "F11", clang-format did something very different this time around... https://codereview.chromium.org/1828103002/diff/1/net/base/parse_number_unitt... net/base/parse_number_unittest.cc:67: {"0", 0}, {"-0", 0}, {"00000", 0}, The difference in these inputs is -0 and other negatives being considered valid.
Actually hold off on this, I want to re-work the interfaces in light of other
callers.
I think I want a more generic version that lets you select the sign of parsing,
as well as determine when there was overflow, since there are callsites which
want to saturate on overflow rather than fail.
I am thinking a parameter driven interface like:
Result ParseInteger(input, <can be negative?>, output);
Where result is {success, parseFailure, underflow, overflow)
Will give the most flexibility.
The ParseNonNegative version is still a commonly used operation that having
shorthands for it makes sense. But shorthands for 'ParseSignedInteger' probably
aren't worthwhile.
On 2016/03/24 15:38:27, eroman wrote:
> Actually hold off on this, I want to re-work the interfaces in light of other
> callers.
>
> I think I want a more generic version that lets you select the sign of
parsing,
> as well as determine when there was overflow, since there are callsites which
> want to saturate on overflow rather than fail.
>
> I am thinking a parameter driven interface like:
>
> Result ParseInteger(input, <can be negative?>, output);
>
> Where result is {success, parseFailure, underflow, overflow)
>
> Will give the most flexibility.
>
> The ParseNonNegative version is still a commonly used operation that having
> shorthands for it makes sense. But shorthands for 'ParseSignedInteger'
probably
> aren't worthwhile.
With all those bools, I assume you're planning on using enum classes?
You'll know when I publish the next patchset :) But yes, I imagine 1 enum for selecting the sign policy, and 1 enum for communicating the result.
On 2016/03/24 15:52:06, eroman wrote: > You'll know when I publish the next patchset :) > > But yes, I imagine 1 enum for selecting the sign policy, and 1 enum for > communicating the result. I suppose it's not as much effort to switch from a bool to an enum when introducing new methods, just thought I'd try to potentially save some wasted effort with an incredibly subtle hint.
Description was changed from ========== Extend net/base/parse_number.h for parsing of signed numbers. There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for int32_t, uint32_t, int64_t, uint64_t. This is the most commonly used function, especially in conjunction with int32_t. * ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t and int64_t. The name includes "Signed" to emphasize that this includes negative numbers. This is not used yet, however will be used soon by some follow-up CLs (certainly the 64-bit version is needed, may not need the 32-bit one though). BUG=596523 ========== to ========== Extend net/base/parse_number.h for parsing of signed numbers. There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for int32_t, uint32_t, int64_t, uint64_t. This is the most commonly used function, especially in conjunction with int32_t. * ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t and int64_t. The name includes "Signed" to emphasize that this includes negative numbers. This is not used yet, however will be used soon by some follow-up CLs (certainly the 64-bit version is needed, may not need the 32-bit one though). BUG=596523,596554 ==========
Description was changed from ========== Extend net/base/parse_number.h for parsing of signed numbers. There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for int32_t, uint32_t, int64_t, uint64_t. This is the most commonly used function, especially in conjunction with int32_t. * ParseSignedInteger() -- parses a decimal number. Has overloads for int32_t and int64_t. The name includes "Signed" to emphasize that this includes negative numbers. This is not used yet, however will be used soon by some follow-up CLs (certainly the 64-bit version is needed, may not need the 32-bit one though). BUG=596523,596554 ========== to ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. The latter is needed in several places to do saturating conversions, and the base::StringToInt() model for determining overflow is broken (callers might think they can check the result on failure against MAX_INT, but that is wrong) There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for all the integer types. * ParseInteger() -- parses a decimal number. Has overloads for all the integer types. Works for both negative and non-negative numbers, and the return value allows caller to determine overflow vs parse error. BUG=596523,596554 ==========
Description was changed from ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. The latter is needed in several places to do saturating conversions, and the base::StringToInt() model for determining overflow is broken (callers might think they can check the result on failure against MAX_INT, but that is wrong) There are now two classes of functions: * ParseNonNegativeInteger() -- parses a decimal number (rejects if negative). Has overloads for all the integer types. * ParseInteger() -- parses a decimal number. Has overloads for all the integer types. Works for both negative and non-negative numbers, and the return value allows caller to determine overflow vs parse error. BUG=596523,596554 ========== to ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. There are now two classes of functions: * ParseInteger() -- parses a decimal number (both negative and positive). Whether negatives are accepted is controlled by an input policy. Returns an enum indicating success/failure/underflow/overflow. * ParseNonNegativeInteger() -- simplified shorthand for the above when parsing non-negative numbers (1*DIGIT). BUG=596523 ==========
Please take another look! To help motivate the new API, see: https://codereview.chromium.org/1827243002/ Something similar is needed in a few places, for instance: https://bugs.chromium.org/p/chromium/issues/detail?id=596561.
https://codereview.chromium.org/1828103002/diff/80001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/80001/net/base/parse_number.h... net/base/parse_number.h:57: FAILED_PARSE, 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.
https://codereview.chromium.org/1828103002/diff/80001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/80001/net/base/parse_number.h... net/base/parse_number.h:57: FAILED_PARSE, On 2016/03/24 21:41:48, mmenke wrote: > 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. 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..
Description was changed from ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. There are now two classes of functions: * ParseInteger() -- parses a decimal number (both negative and positive). Whether negatives are accepted is controlled by an input policy. Returns an enum indicating success/failure/underflow/overflow. * ParseNonNegativeInteger() -- simplified shorthand for the above when parsing non-negative numbers (1*DIGIT). BUG=596523 ========== to ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 ==========
OK, in patchset 6 I have combined it into a single unified function: bool ParseIntegerBase10(input, allow_negatives, *output, *error) I think this addresses your concerns with the previous API.
https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.cc File net/base/parse_number.cc (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.cc:62: } optional: Think this may be clearer as: if (!starts_with_digit) { if (!allow_negative || !starts_with_negative) ... } https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:65: // Whereas when allow_negative==false, accepted inputs may be optionally I think you've switched the true and false cases. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:84: // due to an implementation reason 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. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:104: bool allow_negative, Suggest making this an enum - makes for clearer, if more wordy, callsites. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:105: uint64_t* output, 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. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... File net/base/parse_number_unittest.cc (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:39: T output; output -> expected_output (For all of these)? https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:51: << "Failed to parse: " << test.input; 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. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:53: EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input; Expected should be first https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:73: EXPECT_FALSE(ParseIntegerBase10(input, allow_negative, &result, &error)); 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. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:185: EXPECT_EQ(value, result); 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. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:223: TEST(ParseNumberTest, ParseIntegerBase10Int32) { I guess TEST_P can't handle templates?
https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:65: // Whereas when allow_negative==false, accepted inputs may be optionally On 2016/03/25 15:31:49, mmenke wrote: > I think you've switched the true and false cases. Oops, indeed https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:105: uint64_t* output, On 2016/03/25 15:31:49, mmenke wrote: > 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. 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. https://codereview.chromium.org/1828103002/diff/140001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1828103002/diff/140001/net/base/sdch_manager.... net/base/sdch_manager.cc:386: ports.insert(port); Side-observation: This is currently weird in that it will accept ports larger than 0xFFFF. Not planning to fix this in this CL. Probably the right fix is to consistently use uint16_t for port numbers, and let parsing just enforce correct range on uint16_t here. But there is more work to be done to use uint16_t over int in places that use ports. (But I have noticed that in a number of places we use int for ports. I guess in part the allure of having -1 to mean "unspecified").
https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:105: uint64_t* output, On 2016/03/25 17:08:09, eroman wrote: > On 2016/03/25 15:31:49, mmenke wrote: > > 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. > > 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. I'm fine with the default argument values. I almost suggested them myself, despite having a general preference that we avoid them. https://codereview.chromium.org/1828103002/diff/140001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/1828103002/diff/140001/net/base/sdch_manager.... net/base/sdch_manager.cc:386: ports.insert(port); On 2016/03/25 17:08:09, eroman wrote: > Side-observation: This is currently weird in that it will accept ports larger > than 0xFFFF. > > Not planning to fix this in this CL. > > Probably the right fix is to consistently use uint16_t for port numbers, and let > parsing just enforce correct range on uint16_t here. But there is more work to > be done to use uint16_t over int in places that use ports. (But I have noticed > that in a number of places we use int for ports. I guess in part the allure of > having -1 to mean "unspecified"). I'm actually not sure that's the right behavior...This code has a remarkable lack of failure cases, but I think we'd want to hard-fail in that case. Regardless, beyond the scope of this CL.
Thanks for all the feedback! I think the iterations on design nave definitely been for the better. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.cc File net/base/parse_number.cc (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.cc:62: } On 2016/03/25 15:31:49, mmenke wrote: > optional: Think this may be clearer as: > > if (!starts_with_digit) { > if (!allow_negative || !starts_with_negative) > ... > } That is definitely better. Done. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:65: // Whereas when allow_negative==false, accepted inputs may be optionally On 2016/03/25 17:08:09, eroman wrote: > On 2016/03/25 15:31:49, mmenke wrote: > > I think you've switched the true and false cases. > > Oops, indeed Done - this is no longer applicable. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:84: // due to an implementation reason On 2016/03/25 15:31:49, mmenke wrote: > 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. Done: No longer applicable, due to signed/unsigned split. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:104: bool allow_negative, On 2016/03/25 15:31:49, mmenke wrote: > Suggest making this an enum - makes for clearer, if more wordy, callsites. Done. I named the enum "ParseInteger" to make the callers look nice. But conceptually it is ParseInteger[Policy] https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number.... net/base/parse_number.h:105: uint64_t* output, On 2016/03/25 17:36:32, mmenke wrote: > On 2016/03/25 17:08:09, eroman wrote: > > On 2016/03/25 15:31:49, mmenke wrote: > > > 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. > > > > 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. > > I'm fine with the default argument values. I almost suggested them myself, > despite having a general preference that we avoid them. Ack https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... File net/base/parse_number_unittest.cc (right): https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:39: T output; On 2016/03/25 15:31:50, mmenke wrote: > output -> expected_output (For all of these)? Done. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:51: << "Failed to parse: " << test.input; On 2016/03/25 15:31:49, mmenke wrote: > 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. 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 https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:53: EXPECT_EQ(result, test.output) << "Failed to parse: " << test.input; On 2016/03/25 15:31:50, mmenke wrote: > Expected should be first Done. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:73: EXPECT_FALSE(ParseIntegerBase10(input, allow_negative, &result, &error)); On 2016/03/25 15:31:50, mmenke wrote: > 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. Done (the latter) https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:185: EXPECT_EQ(value, result); On 2016/03/25 15:31:49, mmenke wrote: > 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. 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 '-'. https://codereview.chromium.org/1828103002/diff/120001/net/base/parse_number_... net/base/parse_number_unittest.cc:223: TEST(ParseNumberTest, ParseIntegerBase10Int32) { On 2016/03/25 15:31:49, mmenke wrote: > I guess TEST_P can't handle templates? 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.
A couple nits. I'll take another look at the tests on Monday, and that should be it. Note that you have a couple compile failures. :( https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.cc File net/base/parse_number.cc (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.cc:54: // Numbers must start with either a digit or a negative sign nit: +. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:57: ALLOW_NEGATIVE Think it's worth noting that if this is true, we return FAILED_PARSE on negatives, rather than FAILED_UNDERFLOW. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:63: // too large. nit: suggest high / low instead of large/small. Large is fine, but calling negative numbers "small" to me seems a little unusual. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:86: // negative values can be disallowed by selecting the "selecting the" -> "passing" / "using"?
> Note that you have a couple compile failures. :( Hmm weird. Looks like on mac size_t is a distinct type from either uint32_t or uint64_t..... But if I simply add an overload for size_t it will blow up on other platforms where that is not the case, as now there will be duplicate definitions.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:22: // code can lead to subtle bugs, as the //base versions are more permissive. s/versions are/versions are/ https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:28: // issue applies when parsing a content-length header. s/content-length/Content-Length/ https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:31: // overflow/underflow from parsing failures. Not sure why this matters? https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:47: // This is consistent with the description in RFC 2616 for representing s/2616/7230/ https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:48: // numbers, as well as many other HTTP related standards. 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). https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:91: ParseIntegerError* optional_error = nullptr) 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? https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:98: WARN_UNUSED_RESULT; 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...)
https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:98: WARN_UNUSED_RESULT; On 2016/03/25 22:35:17, Ryan Sleevi wrote: > 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...) "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.
On 2016/03/25 22:49:59, mmenke wrote: > "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. Mentioned on hangouts to Eric, so not trying to pile on, but I think one of the lessons learned is that the integer promotion/conversion rules are subtle. We've presently got a number of the warnings squelched (because of the pervasiveness), but jschuh@ had been on the war path for them. I suppose the argument in favor is that by taking as a pointer, type conversions won't occur - but as Eric notes, this isn't true for typedef'd types (which size_t can be, or it can be a discrete type), which is where my concern originates. I also think it's easier for reviewers to notice when the function name itself gives clear indication as to the size, rather than having to trace through to find the variable declaration (especially if a member variable), but... eh.
Thanks for the feedback Ryan. quick responses to the high-level comments first https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:31: // overflow/underflow from parsing failures. On 2016/03/25 22:35:17, Ryan Sleevi wrote: > Not sure why this matters? 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). https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:91: ParseIntegerError* optional_error = nullptr) On 2016/03/25 22:35:17, Ryan Sleevi wrote: > 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? 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. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:98: WARN_UNUSED_RESULT; On 2016/03/25 22:35:17, Ryan Sleevi wrote: > 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...) 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.
PTAL. I have renamed the stuff to include the type in the name, as in: ParseInt32() ParseUint32() ParseInt64() ParseUint64() https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:22: // code can lead to subtle bugs, as the //base versions are more permissive. On 2016/03/25 22:35:17, Ryan Sleevi wrote: > s/versions are/versions are/ Done. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:28: // issue applies when parsing a content-length header. On 2016/03/25 22:35:17, Ryan Sleevi wrote: > s/content-length/Content-Length/ No longer applicable -I re-wrote this entire comment block to something (hopefully) simpler. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:47: // This is consistent with the description in RFC 2616 for representing On 2016/03/25 22:35:17, Ryan Sleevi wrote: > s/2616/7230/ Done. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:48: // numbers, as well as many other HTTP related standards. On 2016/03/25 22:35:17, Ryan Sleevi wrote: > 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). Done. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:57: ALLOW_NEGATIVE On 2016/03/25 22:10:56, mmenke wrote: > Think it's worth noting that if this is true, we return FAILED_PARSE on > negatives, rather than FAILED_UNDERFLOW. You mean a comment in DISALLOW_NEGATIVE? Done. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:63: // too large. On 2016/03/25 22:10:56, mmenke wrote: > nit: suggest high / low instead of large/small. Large is fine, but calling > negative numbers "small" to me seems a little unusual. Done. https://codereview.chromium.org/1828103002/diff/160001/net/base/parse_number.... net/base/parse_number.h:86: // negative values can be disallowed by selecting the On 2016/03/25 22:10:56, mmenke wrote: > "selecting the" -> "passing" / "using"? Done.
https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.cc File net/base/parse_number.cc (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.cc:76: // You would think these can be used here, but unfortunately they return those nit: Think it's best to avoid personal pronouns like "you". Maybe just remove that clause entirely, and merge with previous paragraph? https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:79: // Returns true on success, and fills |*output| with the result. If Should these be plural, since they refer to all the follow functions? i.e. "These return true on success, and fill..." https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:83: // On failure it is guaranteed that |*output| was not modified. Maybe just: "On failure |*output| will not be modified." for easier parising ("it is" in previous paragraph followed by "On failure it is..." here seems a bit harder to parse) https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:100: NET_EXPORT bool ParseUint32(const base::StringPiece& input, Should these have a separate comment? Don't think it's a huge deal, but the format comment doesn't apply to them. Could even put the uint methods first, and then say "Like the Uint variants, except ParseIntFormat". Anyhow, probably fine as-is. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... File net/base/parse_number_unittest.cc (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:71: const OutputType kBogusOutput = OutputType(23614); Is the OutputType(...) needed? Not entirely sure whether that would be considered a C-style cast or not. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:144: ExpectParseIntFailure<T>(func, IncrementLastChar(std::to_string(value)), Maybe worth a comment that this works because max() of all types will never end in 9? It's just a weird dependency (Though certainly one we can rely on), so may be worth calling out. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:154: const T value = std::numeric_limits<T>::max(); Should we actually make this an overflow as well? We default base behavior is the same in both cases, but may be nice to use a real overflow as well. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:161: for (const auto& format : kFormats) { optional: Rather than all these loops over formats, could just take format as an argument, and loop over them in the fixture. Or could make one big loop...Fine as-is, though. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:163: auto str_value = std::to_string(value); Think it's best to just remove the auto here - think it should generally be avoided unless the type is hideous, or it's a loop variable. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:163: auto str_value = std::to_string(value); Is std::to_string allowed C++0x11? https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:180: IncrementLastChar(std::to_string(std::numeric_limits<T>::min())); Again, there's a weird dependency here...
https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.cc File net/base/parse_number.cc (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.cc:76: // You would think these can be used here, but unfortunately they return those On 2016/04/07 20:42:53, mmenke wrote: > nit: Think it's best to avoid personal pronouns like "you". Maybe just remove > that clause entirely, and merge with previous paragraph? Done. I simplified the various comments in this function. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:79: // Returns true on success, and fills |*output| with the result. If On 2016/04/07 20:42:53, mmenke wrote: > Should these be plural, since they refer to all the follow functions? > > i.e. "These return true on success, and fill..." Reworded the comment blocks. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:83: // On failure it is guaranteed that |*output| was not modified. On 2016/04/07 20:42:53, mmenke wrote: > Maybe just: "On failure |*output| will not be modified." for easier parising > ("it is" in previous paragraph followed by "On failure it is..." here seems a > bit harder to parse) Done. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number.... net/base/parse_number.h:100: NET_EXPORT bool ParseUint32(const base::StringPiece& input, On 2016/04/07 20:42:53, mmenke wrote: > Should these have a separate comment? Don't think it's a huge deal, but the > format comment doesn't apply to them. Could even put the uint methods first, > and then say "Like the Uint variants, except ParseIntFormat". Anyhow, probably > fine as-is. Done. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... File net/base/parse_number_unittest.cc (right): https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:71: const OutputType kBogusOutput = OutputType(23614); On 2016/04/07 20:42:53, mmenke wrote: > Is the OutputType(...) needed? Not entirely sure whether that would be > considered a C-style cast or not. Done. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:144: ExpectParseIntFailure<T>(func, IncrementLastChar(std::to_string(value)), On 2016/04/07 20:42:53, mmenke wrote: > Maybe worth a comment that this works because max() of all types will never end > in 9? It's just a weird dependency (Though certainly one we can rely on), so > may be worth calling out. Extracted to a more specific function. And added assertions rather than comments to document this assumption. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:154: const T value = std::numeric_limits<T>::max(); On 2016/04/07 20:42:53, mmenke wrote: > Should we actually make this an overflow as well? We default base behavior is > the same in both cases, but may be nice to use a real overflow as well. Done. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:161: for (const auto& format : kFormats) { On 2016/04/07 20:42:53, mmenke wrote: > optional: Rather than all these loops over formats, could just take format as > an argument, and loop over them in the fixture. Or could make one big > loop...Fine as-is, though. Done. I also re-unified the tests so there isn't any duplication for the signed vs unsigned flavors. https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:163: auto str_value = std::to_string(value); On 2016/04/07 20:42:53, mmenke wrote: > Is std::to_string allowed C++0x11? Done. You are right, our C++11 guide hasn't taken a stance on this yet. However it is already being used in the codebase... https://codereview.chromium.org/1828103002/diff/220001/net/base/parse_number_... net/base/parse_number_unittest.cc:180: IncrementLastChar(std::to_string(std::numeric_limits<T>::min())); On 2016/04/07 20:42:53, mmenke wrote: > Again, there's a weird dependency here... No longer applicable.
LGTM https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.... net/base/parse_number.h:102: // ParseIntFormat::NON_NEGATIVE. (But defiend for unsigned output types). defiend -> defined https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.... net/base/parse_number.h:102: // ParseIntFormat::NON_NEGATIVE. (But defiend for unsigned output types). The final parenthetical clause is really part of the last sentence. Remove parents and combine with a comma? Or just remove the period and lowercase but?
https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.... net/base/parse_number.h:102: // ParseIntFormat::NON_NEGATIVE. (But defiend for unsigned output types). On 2016/04/08 19:11:06, mmenke wrote: > The final parenthetical clause is really part of the last sentence. Remove > parents and combine with a comma? Or just remove the period and lowercase but? Done. https://codereview.chromium.org/1828103002/diff/260001/net/base/parse_number.... net/base/parse_number.h:102: // ParseIntFormat::NON_NEGATIVE. (But defiend for unsigned output types). On 2016/04/08 19:11:06, mmenke wrote: > defiend -> defined Done.
https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.... net/base/parse_number.h:55: // In other words, this accepts the same things as DISALLOW_NEGATIVE, and I believe this should be NON_NEGATIVE ? https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.... net/base/parse_number.h:84: // On failure it is guaranteed that |*output| was not modified. If s/On failure it/On failure, it/
https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.h File net/base/parse_number.h (right): https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.... net/base/parse_number.h:55: // In other words, this accepts the same things as DISALLOW_NEGATIVE, and On 2016/04/08 19:50:45, Ryan Sleevi wrote: > I believe this should be NON_NEGATIVE ? Done. https://codereview.chromium.org/1828103002/diff/280001/net/base/parse_number.... net/base/parse_number.h:84: // On failure it is guaranteed that |*output| was not modified. If On 2016/04/08 19:50:45, Ryan Sleevi wrote: > s/On failure it/On failure, it/ Done.
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1828103002/#ps300001 (title: "fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828103002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828103002/300001
Message was sent while issue was closed.
Description was changed from ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 ========== to ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 ========== to ========== Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 Committed: https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa Cr-Commit-Position: refs/heads/master@{#386425} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa Cr-Commit-Position: refs/heads/master@{#386425} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
