StringToUint should output zero on negative numbers.

StringToUint should output zero on negative numbers.

The StringToFoo family of functions are documented to, on underflow, write the
minimum supported value (and return false). However, on negative numbers
codepath for unsigned integers, it doesn't write to *output at all. There is
actually a test for this case, but because output is initialized to zero, the
tests don't catch it.

Fix that codepath and make sure the tests notice if StringToUint doesn't write
anything.

BUG=none

Committed: https://crrev.com/301864e18afd79be3857c6101cf856a45016f71f
Cr-Commit-Position: refs/heads/master@{#378057}

[davidben, 2016-02-11 21:36:10]

davidben@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-02-11 21:40:31]

lgtm, but i think there's more if you want:

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
File base/strings/string_number_conversions.cc (right):

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
base/strings/string_number_conversions.cc:143: valid = false;
this also can return false and doesn't write to output

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
base/strings/string_number_conversions.cc:291: return false;
(this also doesn't write to output from what i can tell)

[davidben, 2016-02-11 21:47:22]

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
File base/strings/string_number_conversions.cc (right):

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
base/strings/string_number_conversions.cc:143: valid = false;
On 2016/02/11 21:40:31, Nico wrote:
> this also can return false and doesn't write to output

This just falls through to later, so I think it's fine? Looks like we return
false but will still give a number if there's leading whitespace. Kinda weird,
but fine...

https://codereview.chromium.org/1685743006/diff/1/base/strings/string_number_...
base/strings/string_number_conversions.cc:291: return false;
On 2016/02/11 21:40:31, Nico wrote:
> (this also doesn't write to output from what i can tell)

Hrm. I dunno. I don't have strong opinions about whether this implies it should
write anything in that case:

// Similar to the previous functions, except that output is a vector of bytes.
// |*output| will contain as many bytes as were successfully parsed prior to the
// error.  There is no overflow, but input.size() must be evenly divisible by 2.
// Leading 0x or +/- are not allowed.

The count == 0 case is clearly within spec, although why it returns false
instead of true when count is size is a little puzzling. I would think the empty
string is a valid hex string? Though I'm not comfortable doing anything to that
without digging a bit.

Dunno about the odd case.

[davidben, 2016-02-26 19:27:13]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2016-02-26 19:27:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685743006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685743006/1

[commit-bot: I haz the power, 2016-02-26 22:17:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 22:17:36]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[davidben, 2016-02-26 23:07:38]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-02-26 23:07:54]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2016-02-26 23:09:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685743006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685743006/1

[commit-bot: I haz the power, 2016-02-27 01:31:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-27 01:32:31]

Description was changed from

==========
StringToUint should output zero on negative numbers.

The StringToFoo family of functions are documented to, on underflow, write the
minimum supported value (and return false). However, on negative numbers
codepath for unsigned integers, it doesn't write to *output at all. There is
actually a test for this case, but because output is initialized to zero, the
tests don't catch it.

Fix that codepath and make sure the tests notice if StringToUint doesn't write
anything.

BUG=none
==========

to

==========
StringToUint should output zero on negative numbers.

The StringToFoo family of functions are documented to, on underflow, write the
minimum supported value (and return false). However, on negative numbers
codepath for unsigned integers, it doesn't write to *output at all. There is
actually a test for this case, but because output is initialized to zero, the
tests don't catch it.

Fix that codepath and make sure the tests notice if StringToUint doesn't write
anything.

BUG=none

Committed: https://crrev.com/301864e18afd79be3857c6101cf856a45016f71f
Cr-Commit-Position: refs/heads/master@{#378057}
==========

[commit-bot: I haz the power, 2016-02-27 01:32:32]

Patchset 1 (id:??) landed as
https://crrev.com/301864e18afd79be3857c6101cf856a45016f71f
Cr-Commit-Position: refs/heads/master@{#378057}