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

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

Created:
6 years, 3 months ago by scottmg
Modified:
6 years, 2 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months ago (2013-01-15 17:30:33 UTC) #4
jschuh
lgtm
6 years, 3 months ago (2013-01-15 17:36:08 UTC) #5
scottmg
brettw: could you review as OWNERS?
6 years, 3 months ago (2013-01-15 17:42:07 UTC) #6
scottmg
Over to darin, if you have a chance.
6 years, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months 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, 3 months ago (2013-01-15 22:09:10 UTC) #17
cpu_(ooo_6.6-7.5)
lgtm
6 years, 3 months ago (2013-01-15 23:03:47 UTC) #18
brettw
LGTM
6 years, 3 months 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, 3 months 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, 3 months ago (2013-01-16 17:08:14 UTC) #21
darin (slow to review)
OK, I yield. LGTM
6 years, 3 months 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, 3 months 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, 3 months 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, 2 months ago (2013-01-25 15:40:59 UTC) #25
scottmg
6 years, 2 months 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