HexStringToUInt64 should fail for negative input

Feeding negative numbers to the HexStringToUInt64 should fail, with output 0 (the closest value in the datatype's range).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197614

[Mostyn Bramley-Moore, 2013-04-13 13:46:08]

This patch removes some invalid test cases that were introduced in
https://chromiumcodereview.appspot.com/14103008

[felipeg, 2013-04-13 13:50:23]

LGTM
my fault,
sorry for that

[commit-bot: I haz the power, 2013-04-13 13:54:43]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[brettw, 2013-04-14 04:22:49]

I'm confused, shouldn't the output for these cases be well-defined? It seems
like we should have testcases for all kinds of invalid inputs.

[Mostyn Bramley-Moore, 2013-04-14 07:24:47]

On 2013/04/14 04:22:49, brettw wrote:
> I'm confused, shouldn't the output for these cases be well-defined? It seems
> like we should have testcases for all kinds of invalid inputs.

Turns out the real problem is that c++11 disallows narrowing conversions in
initializer lists:
http://www.stroustrup.com/C++11FAQ.html#narrowing

It would be possible to feed negative values to HexStringToUInt64 if we had a
separate array and didn't use uint64 or didn't use an initializer list.  However
I think these particular test cases are wrong anyway (looks like the entire
block was copy+pasted from the HexStringToInt64 test).

These are the error messages:
../../base/strings/string_number_conversions_unittest.cc: In member function
‘virtual void
base::StringNumberConversionsTest_HexStringToUInt64_Test::TestBody()’:
../../base/strings/string_number_conversions_unittest.cc:357:3: error: narrowing
conversion of ‘-66’ from ‘int’ to ‘uint64 {aka long unsigned int}’ inside { } is
ill-formed in C++11 [-Werror=narrowing]
../../base/strings/string_number_conversions_unittest.cc:357:3: error: narrowing
conversion of ‘-2147483648’ from ‘int’ to ‘uint64 {aka long unsigned int}’
inside { } is ill-formed in C++11 [-Werror=narrowing]
../../base/strings/string_number_conversions_unittest.cc:357:3: error: narrowing
conversion of ‘-66’ from ‘int’ to ‘uint64 {aka long unsigned int}’ inside { } is
ill-formed in C++11 [-Werror=narrowing]
../../base/strings/string_number_conversions_unittest.cc:357:3: error: narrowing
conversion of ‘-2147483648’ from ‘int’ to ‘uint64 {aka long unsigned int}’
inside { } is ill-formed in C++11 [-Werror=narrowing]

[brettw, 2013-04-15 04:36:52]

Seems like we should be able to do "static_cast<uint64>(-66)". Did you try
something like that? If that's not what we expect the output to be (you seem to
imply that you think this is wrong) then can we fix the conversion code?

[Mostyn Bramley-Moore, 2013-04-15 08:44:59]

On 2013/04/15 04:36:52, brettw wrote:
> Seems like we should be able to do "static_cast<uint64>(-66)". Did you try
> something like that? If that's not what we expect the output to be (you seem
to
> imply that you think this is wrong) then can we fix the conversion code?

I changed things around a bit:
* make HexStringToUInt64 fail if fed with negative input
* added back the negative input tests, with static casts (but changed the
expected result to false)
* changed expected results for other negative input to false
* made the test only check result values if HexStringToUInt64 didn't report
failure (aside: this fix should probably also be applied to the HexStringToInt64
test loop)

[brettw, 2013-04-24 05:42:05]

Sorry this fell off my radar, feel free to ping me if I take so many days in the
future.

https://codereview.chromium.org/14109020/diff/7001/base/strings/string_number...
File base/strings/string_number_conversions.cc (right):

https://codereview.chromium.org/14109020/diff/7001/base/strings/string_number...
base/strings/string_number_conversions.cc:504: return false;
The header file defines that the best effort result should saturate (so should
return 0 in the negative cases). Is there something broken in the template code?
It seems like there's a bunch of stuff to test for negative numbers, but I don't
see any unit tests for StringToUint so am suspicious.

[Mostyn Bramley-Moore, 2013-04-24 08:19:20]

https://codereview.chromium.org/14109020/diff/7001/base/strings/string_number...
File base/strings/string_number_conversions.cc (right):

https://codereview.chromium.org/14109020/diff/7001/base/strings/string_number...
base/strings/string_number_conversions.cc:504: return false;
On 2013/04/24 05:42:05, brettw wrote:
> The header file defines that the best effort result should saturate (so should
> return 0 in the negative cases). Is there something broken in the template
code?
> It seems like there's a bunch of stuff to test for negative numbers, but I
don't
> see any unit tests for StringToUint so am suspicious.

The template code only seems to check for valid negative numbers (eg I think it
would spot "-12-12" as invalid), but does not check that the value type is
signed.

I moved the negativity/signed check inside the template code, I guess it's a
more complete fix (in case other unsigned types are added later).

[Mostyn Bramley-Moore, 2013-04-26 08:58:23]

brettw: *ping* :) Any thoughts on patchset 3?

[brettw, 2013-04-27 21:23:42]

Great, that's definitely the right way to check for negative. But the behavior
still doesn't match the header file documentation:

"Overflow/underflow.  |*output| will be set to the maximum value supported by
the data type."

So I think we should also set the output to 0 in that case and update the test
expectations. It is a bit worrying that somebody could be depending on this
behavior, so it seems like the right thing to do.

[Mostyn Bramley-Moore, 2013-04-27 21:46:06]

On 2013/04/27 21:23:42, brettw wrote:
> Great, that's definitely the right way to check for negative. But the behavior
> still doesn't match the header file documentation:
> 
> "Overflow/underflow.  |*output| will be set to the maximum value supported by
> the data type."
> 
> So I think we should also set the output to 0 in that case and update the test
> expectations. It is a bit worrying that somebody could be depending on this
> behavior, so it seems like the right thing to do.

Hmm, but 0 isn't the maximum value supported by the datatype- should I set the
output to -1 instead in this case?

[Mostyn Bramley-Moore, 2013-04-27 23:18:04]

In patchset 4/5 I added back the original test result checking (since we have
defined error output values), and updated the test results.

I also made "-" return failure and 0, since it doesn't count as a parseable
number (as required in the header docs).

Also CC'ing Raphael and Jar, who merged a partial fix in the meantime over in
https://chromiumcodereview.appspot.com/13878015

[brettw, 2013-04-30 21:52:19]

Nice catch for the "-" string input case.

The comment is worded weirdly, since I think it's ambiguous what the "maximum
value" in in this context for an unsigned number.

I asked some people around here and we think that it makes sense to saturate to
the maximum/minumum value of the type in the overflow cases. The old code didn't
handle the negative case at all, so I think that's where the comment author was
coming from.

So I still think we should saturate to 0, and we should update the .h file to
say it will clamp to the minumum/maxiumum value of the type. Do you agree?

[Mostyn Bramley-Moore, 2013-04-30 23:14:55]

Sounds OK to me- done in patchset 6.  We lose the ability to distinguish between
unparseable input and underflow, but it still fails and fits the pattern of the
other functions.

Note however that I think that more work is required to recognise underflow for
signed types and set the minimum value there.  Would you prefer this to be done
as part of this patch, or should we merge this and a new patch?

[brettw, 2013-04-30 23:26:29]

LGTM.

Let's do any additional work in a separate patch, this one has dragged on long
enough and I think is a more important fix.

[commit-bot: I haz the power, 2013-04-30 23:41:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/14109020/24001

[commit-bot: I haz the power, 2013-05-01 00:01:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-05-01 05:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/14109020/24001

[commit-bot: I haz the power, 2013-05-01 13:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/14109020/24001

[commit-bot: I haz the power, 2013-05-01 14:22:17]

Change committed as 197614