Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(31)

Issue 11886037: Add numeric_cast for checked integral narrowing casts (Closed)

Created:
6 years, 1 month ago by scottmg
Modified:
6 years ago
CC:
chromium-reviews, erikwright+watch_chromium.org
Visibility:
Public.

Description

Add numeric_cast for checked integral narrowing casts In work on bringing up Windows x64, there are many places that need to be safely narrowed to the types used for interacting with other APIs (particularly when using containers). Rather than scatter these CHECKs all over, numeric_cast<> CHECKs that the runtime value can be safely converted to the target type. BUG=8606, 167187, 166496 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177264

Patch Set 1 #

Patch Set 2 : review fixes #

Patch Set 3 : rename to safe_numerics #

Total comments: 15

Patch Set 4 : review fixes #

Total comments: 2

Patch Set 5 : -> checked_numeric_cast #

Patch Set 6 : add some more test cases #

Patch Set 7 : initial attempt via template specialization #

Patch Set 8 : macroize to reduce noise #

Patch Set 9 : remove temp code #

Patch Set 10 : add exhaustive test, fix comment #

Patch Set 11 : add exhaustive test, fix comment #

Patch Set 12 : rename macro #

Patch Set 13 : simplify #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+306 lines, -0 lines) Patch
M base/base.gyp View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A base/safe_numerics.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +135 lines, -0 lines 1 comment Download
A base/safe_numerics_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +151 lines, -0 lines 1 comment Download
A base/safe_numerics_unittest.nc View 1 2 3 4 5 6 1 chunk +17 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
scottmg
Would you mind taking a look before I send to brett|darin?
6 years, 1 month ago (2013-01-15 05:23:05 UTC) #1
jschuh
On 2013/01/15 05:23:05, scottmg wrote: > Would you mind taking a look before I send ...
6 years, 1 month ago (2013-01-15 09:10:14 UTC) #2
scottmg
On 2013/01/15 09:10:14, Justin Schuh wrote: > On 2013/01/15 05:23:05, scottmg wrote: > > Would ...
6 years, 1 month ago (2013-01-15 16:29:18 UTC) #3
scottmg
On 2013/01/15 09:10:14, Justin Schuh wrote: > Also, we're going to need math operations too. ...
6 years, 1 month ago (2013-01-15 17:30:33 UTC) #4
jschuh
lgtm
6 years, 1 month ago (2013-01-15 17:36:08 UTC) #5
scottmg
brettw: could you review as OWNERS?
6 years, 1 month ago (2013-01-15 17:42:07 UTC) #6
scottmg
Over to darin, if you have a chance.
6 years, 1 month ago (2013-01-15 18:33:37 UTC) #7
darin (slow to review)
https://codereview.chromium.org/11886037/diff/11001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/11886037/diff/11001/base/base.gyp#newcode13 base/base.gyp:13: 'targets': [ safe_numerics.h should be added to base.gypi https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h ...
6 years, 1 month ago (2013-01-15 18:53:28 UTC) #8
jschuh
https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newcode5 base/safe_numerics.h:5: #ifndef BASE_NUMERIC_CAST_H_ On 2013/01/15 18:53:29, darin wrote: > nit: ...
6 years, 1 month ago (2013-01-15 19:07:32 UTC) #9
darin (slow to review)
OK, makes sense! On Tue, Jan 15, 2013 at 11:07 AM, <jschuh@chromium.org> wrote: > > ...
6 years, 1 month ago (2013-01-15 19:14:58 UTC) #10
scottmg
Thanks! https://codereview.chromium.org/11886037/diff/11001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/11886037/diff/11001/base/base.gyp#newcode13 base/base.gyp:13: 'targets': [ On 2013/01/15 18:53:29, darin wrote: > ...
6 years, 1 month ago (2013-01-15 19:30:17 UTC) #11
brettw
Wondering out loud: what about calling this something like checked_numeric_cast instead? One could imagine a ...
6 years, 1 month ago (2013-01-15 21:09:50 UTC) #12
jschuh
https://codereview.chromium.org/11886037/diff/23001/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/11886037/diff/23001/base/safe_numerics.h#newcode57 base/safe_numerics.h:57: } // namespace base On 2013/01/15 21:09:50, brettw wrote: ...
6 years, 1 month ago (2013-01-15 21:13:27 UTC) #13
brettw
saturating_cast SGTM, although I still wonder if we need "checked" since the current name doesn't ...
6 years, 1 month ago (2013-01-15 21:15:47 UTC) #14
scottmg
On 2013/01/15 21:09:50, brettw wrote: > Wondering out loud: what about calling this something like ...
6 years, 1 month ago (2013-01-15 21:47:06 UTC) #15
scottmg
On 2013/01/15 21:47:06, scottmg wrote: > On 2013/01/15 21:09:50, brettw wrote: > > Wondering out ...
6 years, 1 month ago (2013-01-15 21:50:31 UTC) #16
darin (slow to review)
LGTM +1 to Brett's suggestion of instantiating common templates.
6 years, 1 month ago (2013-01-15 22:09:10 UTC) #17
cpu_(ooo_6.6-7.5)
lgtm
6 years, 1 month ago (2013-01-15 23:03:47 UTC) #18
brettw
LGTM
6 years, 1 month ago (2013-01-16 00:11:46 UTC) #19
scottmg
Unfortunately, the patch that everyone reviewed (patchset 5) only worked on VS and gcc. clang ...
6 years, 1 month ago (2013-01-16 16:19:49 UTC) #20
jschuh
On 2013/01/16 16:19:49, scottmg wrote: > Unfortunately, the patch that everyone reviewed (patchset 5) only ...
6 years, 1 month ago (2013-01-16 17:08:14 UTC) #21
darin (slow to review)
OK, I yield. LGTM
6 years, 1 month ago (2013-01-16 18:29:08 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/11886037/20
6 years, 1 month ago (2013-01-16 20:10:25 UTC) #23
commit-bot: I haz the power
Retried try job too often on win7_aura for step(s) content_browsertests
6 years, 1 month ago (2013-01-16 23:31:03 UTC) #24
wtc
https://codereview.chromium.org/11886037/diff/20/base/safe_numerics.h File base/safe_numerics.h (right): https://codereview.chromium.org/11886037/diff/20/base/safe_numerics.h#newcode123 base/safe_numerics.h:123: // numeric_cast<> is analogous to static_cast<> for numeric types, ...
6 years ago (2013-01-25 15:40:59 UTC) #25
scottmg
6 years ago (2013-01-25 16:10:47 UTC) #26
Message was sent while issue was closed.
On 2013/01/25 15:40:59, wtc wrote:
> https://codereview.chromium.org/11886037/diff/20/base/safe_numerics.h
> File base/safe_numerics.h (right):
> 
>
https://codereview.chromium.org/11886037/diff/20/base/safe_numerics.h#newcode123
> base/safe_numerics.h:123: // numeric_cast<> is analogous to static_cast<> for
> numeric types, except that
> 
> The cast is checked_numeric_cast not numeric_cast, right?
> 
>
https://codereview.chromium.org/11886037/diff/20/base/safe_numerics_unittest.cc
> File base/safe_numerics_unittest.cc (right):
> 
>
https://codereview.chromium.org/11886037/diff/20/base/safe_numerics_unittest....
> base/safe_numerics_unittest.cc:133: // Confirm that numeric_cast<> actually
> compiles.
> 
> Same here.

Thanks, https://codereview.chromium.org/12051089/

Powered by Google App Engine
This is Rietveld 408576698