Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14794002:
add more string -> unsigned number conversion unit tests (Closed)
|
Created:
7 years, 7 months ago by Mostyn Bramley-Moore Modified:
7 years, 7 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionadd more string -> unsigned number conversion unit tests
Add unit tests for the following functions in base:
StringToUint
StringToUint64
StringToSizeT
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201204
Patch Set 1 #Patch Set 2 : size_t cast fixups #
Total comments: 2
Patch Set 3 : special case for 32bit size_t #Patch Set 4 : remove redundant 64bit size_t test #Patch Set 5 : attempt to disable windows warning, let's see if it really works #
Total comments: 2
Patch Set 6 : SIZE_MAX defines the limit of size_t #
Total comments: 2
Patch Set 7 : attempt to remove unrequired static casts #Messages
Total messages: 23 (0 generated)
@Brettw: is it worth adding more unsigned number conversion tests?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/14794002/1
Patchset 2 has some casting fixups, does this need to be OK'ed, or should I just re-tick the commit box?
@brettw: ping
https://codereview.chromium.org/14794002/diff/16001/base/strings/string_numbe... File base/strings/string_number_conversions_unittest.cc (right): https://codereview.chromium.org/14794002/diff/16001/base/strings/string_numbe... base/strings/string_number_conversions_unittest.cc:347: {"99999999999", static_cast<size_t>(99999999999), true}, it cant work on 32bit platform where size_t 32bit
https://codereview.chromium.org/14794002/diff/16001/base/strings/string_numbe... File base/strings/string_number_conversions_unittest.cc (right): https://codereview.chromium.org/14794002/diff/16001/base/strings/string_numbe... base/strings/string_number_conversions_unittest.cc:347: {"99999999999", static_cast<size_t>(99999999999), true}, Thanks for the review. On 2013/05/18 08:55:17, Vitaly Buka corp wrote: > it cant work on 32bit platform where size_t 32bit Done. Moved to a special case further down.
lgtm
On 2013/05/20 18:38:40, Vitaly Buka corp wrote: > lgtm It still fails on windows. Same as above true for 2147483648 Please fix and make sure try bots are green.
On 2013/05/20 18:40:52, Vitaly Buka corp wrote: > On 2013/05/20 18:38:40, Vitaly Buka corp wrote: > > lgtm > > It still fails on windows. > Same as above true for 2147483648 According to this page[1], size_t should be 32bits on 32bit windows and 64 bits on 64bit windows, so I suspect that the code is actually correct and the cast will be optimized away (or at least never called). Would a warning suppression be sufficient? [1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx > Please fix and make sure try bots are green. I'm not sure if it's possible for me to submit try jobs without using the CQ- is it OK for me to tick the commit box after submitting another patchset without waiting for another thumbs-up?
How did you run bots "Patch Set 1"? On 2013/05/20 19:02:25, Mostyn Bramley-Moore wrote: > On 2013/05/20 18:40:52, Vitaly Buka corp wrote: > > On 2013/05/20 18:38:40, Vitaly Buka corp wrote: > > > lgtm > > > > It still fails on windows. > > Same as above true for 2147483648 > > According to this page[1], size_t should be 32bits on 32bit windows and 64 bits > on 64bit windows, so I suspect that the code is actually correct and the cast > will be optimized away (or at least never called). Would a warning suppression > be sufficient? > > [1] > http://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29... > > > Please fix and make sure try bots are green. > > I'm not sure if it's possible for me to submit try jobs without using the CQ- is > it OK for me to tick the commit box after submitting another patchset without > waiting for another thumbs-up?
On 2013/05/20 19:19:37, Vitaly Buka corp wrote: > How did you run bots "Patch Set 1"? It was a couple of weeks ago so difficult to remember, I think either brettw or I ticked the commit box after brettw gave the first "lgtm". (I asked in the irc channel just now and thakis queued some new try jobs for me...)
lgtm https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... File base/strings/string_number_conversions_unittest.cc (right): https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... base/strings/string_number_conversions_unittest.cc:390: #endif Can you put this tests back to array and use #if !defined(ARCH_CPU_X86_64) to disable them there
On 2013/05/20 19:42:49, Vitaly Buka corp wrote: > lgtm please ignore this lgtm, clicked accidentally.
https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... File base/strings/string_number_conversions_unittest.cc (right): https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... base/strings/string_number_conversions_unittest.cc:390: #endif On 2013/05/20 19:42:49, Vitaly Buka corp wrote: > Can you put this tests back to array and > use #if !defined(ARCH_CPU_X86_64) to disable them there I found SIZE_MAX in stdint.h which should be more correct, so I used "#if SIZE_MAX > 4294967295U" instead...
ok On 2013/05/20 20:13:34, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... > File base/strings/string_number_conversions_unittest.cc (right): > > https://codereview.chromium.org/14794002/diff/33001/base/strings/string_numbe... > base/strings/string_number_conversions_unittest.cc:390: #endif > On 2013/05/20 19:42:49, Vitaly Buka corp wrote: > > Can you put this tests back to array and > > use #if !defined(ARCH_CPU_X86_64) to disable them there > > I found SIZE_MAX in stdint.h which should be more correct, so I used "#if > SIZE_MAX > 4294967295U" instead...
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/14794002/29003
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... 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.
https://chromiumcodereview.appspot.com/14794002/diff/29003/base/strings/strin... File base/strings/string_number_conversions_unittest.cc (right): https://chromiumcodereview.appspot.com/14794002/diff/29003/base/strings/strin... base/strings/string_number_conversions_unittest.cc:348: {"2147483648", static_cast<size_t>(2147483648), true}, try 2147483648U and 99999999999U probably you also do not need static_cast in this case
https://chromiumcodereview.appspot.com/14794002/diff/29003/base/strings/strin... File base/strings/string_number_conversions_unittest.cc (right): https://chromiumcodereview.appspot.com/14794002/diff/29003/base/strings/strin... base/strings/string_number_conversions_unittest.cc:348: {"2147483648", static_cast<size_t>(2147483648), true}, On 2013/05/20 21:29:11, Vitaly Buka wrote: > try 2147483648U and 99999999999U > probably you also do not need static_cast in this case Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/14794002/46001
Message was sent while issue was closed.
Change committed as 201204 |
