Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
187793003:
Define print format macros for NSInteger & NSUInteger (Closed)
|
Created:
6 years, 9 months ago by sdefresne Modified:
6 years, 9 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDefine print format macros for NSInteger & NSUInteger
The size of NSInteger and NSUInteger varies between 32-bit and 64-bit
architectures, however does not provides macro to safely format them
and instead recommend casting the value to the larger version that is
used on 64-bit architecture.
Using a cast could cause some formatting to be missed (if the type of
a variable changes), so instead we define our own macros to format those
types safely.
BUG=349458
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255755
Patch Set 1 #Patch Set 2 : Re-upload #
Total comments: 6
Patch Set 3 : Improve unit tests & fix grammatical error #
Total comments: 8
Patch Set 4 : Fix comments from reviewer #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase and try CQ again #Messages
Total messages: 31 (0 generated)
Please take a look.
error: old chunk mismatch Please try your upload again. Rietveld is kind of a mess.
On 2014/03/05 18:42:39, Mark Mentovai wrote: > error: old chunk mismatch > > Please try your upload again. Rietveld is kind of a mess. Done. PTAL.
https://codereview.chromium.org/187793003/diff/20001/base/format_macros.h File base/format_macros.h (right): https://codereview.chromium.org/187793003/diff/20001/base/format_macros.h#new... base/format_macros.h:51: // recommends casting. This has many drawback, so instead defines macros defines->define https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... File base/mac/foundation_util_unittest.mm (right): https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:322: EXPECT_EQ("12345678", StringPrintf("%" PRIdNS, kNSInteger)); Test that PRIdNS formats negative signed numbers properly, and PRIuNS formats high-bit-set unsigned numbers properly. https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:325: } #ifdef __LP64__, do a couple of tests with things that would only fit in a 32-bit quantity.
PTAL https://codereview.chromium.org/187793003/diff/20001/base/format_macros.h File base/format_macros.h (right): https://codereview.chromium.org/187793003/diff/20001/base/format_macros.h#new... base/format_macros.h:51: // recommends casting. This has many drawback, so instead defines macros On 2014/03/06 14:44:57, Mark Mentovai wrote: > defines->define Done. https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... File base/mac/foundation_util_unittest.mm (right): https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:322: EXPECT_EQ("12345678", StringPrintf("%" PRIdNS, kNSInteger)); On 2014/03/06 14:44:57, Mark Mentovai wrote: > Test that PRIdNS formats negative signed numbers properly, and PRIuNS formats > high-bit-set unsigned numbers properly. Done. https://codereview.chromium.org/187793003/diff/20001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:325: } On 2014/03/06 14:44:57, Mark Mentovai wrote: > #ifdef __LP64__, do a couple of tests with things that would only fit in a > 32-bit quantity. Done.
LGTM with these changes. https://codereview.chromium.org/187793003/diff/40001/base/format_macros.h File base/format_macros.h (right): https://codereview.chromium.org/187793003/diff/40001/base/format_macros.h#new... base/format_macros.h:51: // recommends casting. This has many drawback, so instead define macros drawback -> drawbacks https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... File base/mac/foundation_util_unittest.mm (right): https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:334: FormatNSIntegerAsType* pointer_to_some_nsinteger = &some_nsinteger; You could have also used UNUSED from base/compiler_specific.h in this declaration instead of ignore_result and a separate expression. Up to you. https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:342: struct { const? Same on line 363. https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:371: {137451299150u, "137451299150", "2000bc614e"}, ul, not just u, right?
Thank you for the review. https://codereview.chromium.org/187793003/diff/40001/base/format_macros.h File base/format_macros.h (right): https://codereview.chromium.org/187793003/diff/40001/base/format_macros.h#new... base/format_macros.h:51: // recommends casting. This has many drawback, so instead define macros On 2014/03/06 17:02:42, Mark Mentovai wrote: > drawback -> drawbacks Done. https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... File base/mac/foundation_util_unittest.mm (right): https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:334: FormatNSIntegerAsType* pointer_to_some_nsinteger = &some_nsinteger; On 2014/03/06 17:02:42, Mark Mentovai wrote: > You could have also used UNUSED from base/compiler_specific.h in this > declaration instead of ignore_result and a separate expression. Up to you. Done. https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:342: struct { On 2014/03/06 17:02:42, Mark Mentovai wrote: > const? Same on line 363. Done. https://codereview.chromium.org/187793003/diff/40001/base/mac/foundation_util... base/mac/foundation_util_unittest.mm:371: {137451299150u, "137451299150", "2000bc614e"}, On 2014/03/06 17:02:42, Mark Mentovai wrote: > ul, not just u, right? Done.
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg, mac_chromium_rel
On 2014/03/06 18:33:58, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: mac_chromium_compile_dbg, > mac_chromium_rel Fails on the revert steps, before even touching my change. Is the CQ broken tonight?
We should ask chrome-infrastructure-team.
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg, mac_chromium_rel
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
The CQ bit was checked by sdefresne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sdefresne@chromium.org/187793003/120001
Message was sent while issue was closed.
Change committed as 255755
Message was sent while issue was closed.
On 2014/03/08 11:43:17, I haz the power (commit-bot) wrote: > Change committed as 255755 This change appears to have caused failures on the Mac ASAN bots: http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%... Reverted in r255781 |
