add more string -> unsigned number conversion unit tests (attempt 2)

add more string -> unsigned number conversion unit tests (attempt 2)

Add unit tests for the following functions in base:
StringToUint
StringToUint64
StringToSizeT

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

[Mostyn Bramley-Moore, 2013-05-21 06:01:00]

This was committed in https://chromiumcodereview.appspot.com/14794002/ and
reverted in https://codereview.chromium.org/15464016/

The first patchset is the originally merged patch, patchset 2 contains the
proposed fix.  Can someone please run a "Google Chrome Linux" try job for me?

[Mostyn Bramley-Moore, 2013-05-21 12:45:58]

The linux_rel_precise32 try job passed- I assume this is the closest to the
"Google Chrome Linux" buildbot.  Anyone willing to give this an "lgtm" and see
if it sticks this time?

[Vitaly Buka (NO REVIEWS), 2013-05-21 18:54:34]

So the only difference with reverted one is 
GG_INT64_C -> GG_UINT64_C

lgtm, but I am not OWNER

[Mostyn Bramley-Moore, 2013-05-21 19:00:25]

On 2013/05/21 18:54:34, Vitaly Buka wrote:
> So the only difference with reverted one is 
> GG_INT64_C -> GG_UINT64_C

The failure is fixed by line 304:
9223372036854775808U -> GG_UINT64_C(<num without the 'U'>)

The GG_INT64_C -> GG_UINT64_C lines didn't fail previously, but I believe this
is more correct.

> lgtm, but I am not OWNER

@brettw - does this look good to you?

[Mostyn Bramley-Moore, 2013-05-23 23:29:12]

@jar: are you able to review this?  

Patchset 1 was committed in https://chromiumcodereview.appspot.com/14794002/ and
then
reverted in https://codereview.chromium.org/15464016/ due to the "Google Chrome
Linux" buildbot failing.

Patchset 2 contains what I believe should fix the problem.

[Mostyn Bramley-Moore, 2013-06-04 21:51:50]

Ping.  Brettw, jar: does this updated patch look OK?

[jar (doing other things), 2013-06-04 22:15:46]

LGTM with one nit below.

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
base/strings/string_number_conversions_unittest.cc:372:
{"999999999999999999999999", std::numeric_limits<size_t>::max(), false},
nit: you could probably do a test for this by using printf to generate the
string... and then converting it back.

You seem to always test the limits in the other case... may as well do it for
this case.

[Mostyn Bramley-Moore, 2013-06-04 23:08:56]

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
base/strings/string_number_conversions_unittest.cc:372:
{"999999999999999999999999", std::numeric_limits<size_t>::max(), false},
On 2013/06/04 22:15:46, jar wrote:
> nit: you could probably do a test for this by using printf to generate the
> string... and then converting it back.
> 
> You seem to always test the limits in the other case... may as well do it for
> this case.

Do you mean sprintf std::numeric_limits<size_t>::max() to a string, then run
that string back through StringToSizeT ?

[jar (doing other things), 2013-06-04 23:46:16]

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
base/strings/string_number_conversions_unittest.cc:372:
{"999999999999999999999999", std::numeric_limits<size_t>::max(), false},
On 2013/06/04 23:08:56, Mostyn Bramley-Moore wrote:
> On 2013/06/04 22:15:46, jar wrote:
> > nit: you could probably do a test for this by using printf to generate the
> > string... and then converting it back.
> > 
> > You seem to always test the limits in the other case... may as well do it
for
> > this case.
> 
> Do you mean sprintf std::numeric_limits<size_t>::max() to a string, then run
> that string back through StringToSizeT ?

Yes. sprintf.

[Mostyn Bramley-Moore, 2013-06-05 14:55:00]

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/2001/base/strings/string_number...
base/strings/string_number_conversions_unittest.cc:372:
{"999999999999999999999999", std::numeric_limits<size_t>::max(), false},
On 2013/06/04 23:46:16, jar wrote:
> On 2013/06/04 23:08:56, Mostyn Bramley-Moore wrote:
> > On 2013/06/04 22:15:46, jar wrote:
> > > nit: you could probably do a test for this by using printf to generate the
> > > string... and then converting it back.
> > > 
> > > You seem to always test the limits in the other case... may as well do it
> for
> > > this case.
> > 
> > Do you mean sprintf std::numeric_limits<size_t>::max() to a string, then run
> > that string back through StringToSizeT ?
> 
> Yes. sprintf.

Done in patchset 3.

[jar (doing other things), 2013-06-06 16:00:00]

lgtm

[commit-bot: I haz the power, 2013-06-06 20:52:21]

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

[commit-bot: I haz the power, 2013-06-06 21:18:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Mostyn Bramley-Moore, 2013-06-06 21:32:41]

I don't have android toolchains setup locally, but I think switching from C
log10 to C++ std::log10 might fix the android clang build error.

@Jar: patchset 4 look ok?

[jar (doing other things), 2013-06-10 03:45:49]

On 2013/06/06 21:32:41, Mostyn Bramley-Moore wrote:
> I don't have android toolchains setup locally, but I think switching from C
> log10 to C++ std::log10 might fix the android clang build error.
> 
> @Jar: patchset 4 look ok?

Patch set 4 LGTM.

[commit-bot: I haz the power, 2013-06-10 06:27:37]

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

[commit-bot: I haz the power, 2013-06-10 06:43:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Mostyn Bramley-Moore, 2013-06-10 06:50:01]

Patchset 5 should avoid the ambiguous overloaded call.  Does this look OK to
resubmit (I wish I had access to the try servers so I could test this before
asking)?

[Mostyn Bramley-Moore, 2013-06-11 06:32:49]

@jar: after some messing around (I don't have easy access to a windows
development machine), I think patchset 11 should work.  The try jobs failed but
for what look like unrelated issues.

[jar (doing other things), 2013-06-12 18:24:27]

nit below... still LGTM

https://codereview.chromium.org/15521004/diff/74002/base/strings/string_numbe...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/74002/base/strings/string_numbe...
base/strings/string_number_conversions_unittest.cc:346:
base::snprintf(size_t_max_cstr, cstr_len, "%" PRIuS, size_t_max);
There is a method for sprint'ing into a std::string.  You can use that, and then
access the cstr() in it to run the tests.  This way you don't need to new up an
array, and delete it.

If you enamored with the new char*, put in into a scoped_ptr, so that you don't
need any explicit delete.

[Mostyn Bramley-Moore, 2013-06-12 20:29:16]

https://codereview.chromium.org/15521004/diff/74002/base/strings/string_numbe...
File base/strings/string_number_conversions_unittest.cc (right):

https://codereview.chromium.org/15521004/diff/74002/base/strings/string_numbe...
base/strings/string_number_conversions_unittest.cc:346:
base::snprintf(size_t_max_cstr, cstr_len, "%" PRIuS, size_t_max);
On 2013/06/12 18:24:27, jar wrote:
> There is a method for sprint'ing into a std::string.  You can use that, and
then
> access the cstr() in it to run the tests.  This way you don't need to new up
an
> array, and delete it.

Done.

This also allows us to drop the string size calculation.

[Mostyn Bramley-Moore, 2013-06-12 20:33:53]

Hmm, and we don't need c_str() - the input is std::string already.

[commit-bot: I haz the power, 2013-06-12 21:33:57]

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

[commit-bot: I haz the power, 2013-06-13 02:27:37]

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

[commit-bot: I haz the power, 2013-06-13 09:04:39]

Change committed as 206031