Add numeric_cast for checked integral narrowing casts

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

[scottmg, 2013-01-15 05:23:05]

Would you mind taking a look before I send to brett|darin?

[jschuh, 2013-01-15 09:10:14]

On 2013/01/15 05:23:05, scottmg wrote:
> Would you mind taking a look before I send to brett|darin?

You can reduce the number of checks and get rid of the warnings like this:
https://codereview.chromium.org/11881047

Also, we're going to need math operations too. Maybe we should just have a
safe_numerics.h or something, and put numeric_cast in there?

[scottmg, 2013-01-15 16:29:18]

On 2013/01/15 09:10:14, Justin Schuh wrote:
> On 2013/01/15 05:23:05, scottmg wrote:
> > Would you mind taking a look before I send to brett|darin?
> 
> You can reduce the number of checks and get rid of the warnings like this:
> https://codereview.chromium.org/11881047

Thanks! Done.

> 
> Also, we're going to need math operations too. Maybe we should just have a
> safe_numerics.h or something, and put numeric_cast in there?

Hmm, OK. How about we just move it once we have a need for those? (rather than
giving this little check an overly grand name for now)

[scottmg, 2013-01-15 17:30:33]

On 2013/01/15 09:10:14, Justin Schuh wrote:
> Also, we're going to need math operations too. Maybe we should just have a
> safe_numerics.h or something, and put numeric_cast in there?

OK, renamed in preparation for you to slam all your other crazy crap in here.

[jschuh, 2013-01-15 17:36:08]

lgtm

[scottmg, 2013-01-15 17:42:07]

brettw: could you review as OWNERS?

[scottmg, 2013-01-15 18:33:37]

Over to darin, if you have a chance.

[darin (slow to review), 2013-01-15 18:53:28]

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
File base/safe_numerics.h (right):

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:5: #ifndef BASE_NUMERIC_CAST_H_
nit: fix include guard.  or maybe the file name numeric_cast.h would be better? 
afterall, that is the name of the method being defined.  usually, it works best
if the file name and key class or function name match.  helps people remember
what to include or what to call.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:20: template <class Dest, class Source>
this feels like implementation detail.  perhaps it should live in the "internal"
namespace so that people don't use it directly.  that is, i assume you mean for
the public API to just be the numeric_cast<> function, right?

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:21: inline bool IsNumericCastableTo(Source source) {
nit: IsValidNumericCast?

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:22: typedef std::numeric_limits<Source> source_limits;
nit: typedefs are usually MixedCase... SourceLimits and DestLimits

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
File base/safe_numerics_unittest.cc (right):

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
base/safe_numerics_unittest.cc:11: TEST(SafeNumerics, NumericCast) {
nit: put this TEST inside the base namespace so that you don't need to type
base:: everywhere.  if you follow my suggestion of putting IsValidNumericCast
into the base::internal:: namespace, then you could also put the TEST in the
base::internal:: namespace too or just add a using directive for
IsValidNumericCast.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
base/safe_numerics_unittest.cc:68: //
EXPECT_TRUE(base::IsNumericCastableTo<float>(0.0));
I think there is support for testing that certain code won't compile.  See
build/nocompile.gypi and base/bind_unittest.nc and base/callback_unittest.nc

[jschuh, 2013-01-15 19:07:32]

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#newc...
base/safe_numerics.h:5: #ifndef BASE_NUMERIC_CAST_H_
On 2013/01/15 18:53:29, darin wrote:
> nit: fix include guard.  or maybe the file name numeric_cast.h would be
better? 
> afterall, that is the name of the method being defined.  usually, it works
best
> if the file name and key class or function name match.  helps people remember
> what to include or what to call.

This was my request. I want to put the safe_numeric classes in this file. The
cast operation is just the easiest one to implement, and the one we need asap.

[darin (slow to review), 2013-01-15 19:14:58]

OK, makes sense!

On Tue, Jan 15, 2013 at 11:07 AM, <jschuh@chromium.org> wrote:

>
> https://codereview.chromium.**org/11886037/diff/11001/base/**
>
safe_numerics.h<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<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: fix include guard.  or maybe the file name numeric_cast.h would
>>
> be better?
>
>> afterall, that is the name of the method being defined.  usually, it
>>
> works best
>
>> if the file name and key class or function name match.  helps people
>>
> remember
>
>> what to include or what to call.
>>
>
> This was my request. I want to put the safe_numeric classes in this
> file. The cast operation is just the easiest one to implement, and the
> one we need asap.
>
>
https://codereview.chromium.**org/11886037/<https://codereview.chromium.org/1...
>

[scottmg, 2013-01-15 19:30:17]

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:
> safe_numerics.h should be added to base.gypi

Done.

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#newc...
base/safe_numerics.h:5: #ifndef BASE_NUMERIC_CAST_H_
On 2013/01/15 18:53:29, darin wrote:
> nit: fix include guard.  or maybe the file name numeric_cast.h would be
better? 
> afterall, that is the name of the method being defined.  usually, it works
best
> if the file name and key class or function name match.  helps people remember
> what to include or what to call.

Justin wanted it called safe_numerics with the intention that there would be
more things (other math operations) added to it soon.

I'm ambivalent either way if separate/narrow headers seem better.

(Fixed include guard for now)

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:20: template <class Dest, class Source>
On 2013/01/15 18:53:29, darin wrote:
> this feels like implementation detail.  perhaps it should live in the
"internal"
> namespace so that people don't use it directly.  that is, i assume you mean
for
> the public API to just be the numeric_cast<> function, right?

Done.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:21: inline bool IsNumericCastableTo(Source source) {
On 2013/01/15 18:53:29, darin wrote:
> nit: IsValidNumericCast?

Done.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics.h#newc...
base/safe_numerics.h:22: typedef std::numeric_limits<Source> source_limits;
On 2013/01/15 18:53:29, darin wrote:
> nit: typedefs are usually MixedCase... SourceLimits and DestLimits

Done.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
File base/safe_numerics_unittest.cc (right):

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
base/safe_numerics_unittest.cc:11: TEST(SafeNumerics, NumericCast) {
On 2013/01/15 18:53:29, darin wrote:
> nit: put this TEST inside the base namespace so that you don't need to type
> base:: everywhere.  if you follow my suggestion of putting IsValidNumericCast
> into the base::internal:: namespace, then you could also put the TEST in the
> base::internal:: namespace too or just add a using directive for
> IsValidNumericCast.

Done.

https://codereview.chromium.org/11886037/diff/11001/base/safe_numerics_unitte...
base/safe_numerics_unittest.cc:68: //
EXPECT_TRUE(base::IsNumericCastableTo<float>(0.0));
On 2013/01/15 18:53:29, darin wrote:
> I think there is support for testing that certain code won't compile.  See
> build/nocompile.gypi and base/bind_unittest.nc and base/callback_unittest.nc

Looks like they've been disabled for a while unfortunately. :(
https://code.google.com/p/chromium/issues/detail?id=105388

Moved to base/safe_numerics_unittest.nc as examples anyway.

[brettw, 2013-01-15 21:09:50]

Wondering out loud: what about calling this something like checked_numeric_cast
instead? One could imagine a saturating_numeric_cast or something for other use
cases.

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#newc...
base/safe_numerics.h:57: }  // namespace base
We've done some extern templates in some cases which GCC supports to avoid
instantiating these in every module and slowing down the compile (see logging.h)
I wonder if it's worthwhile to define some of these for the several types that
will cover 99% of the use cases. Don't feel like you need to do it in this
patch.

[jschuh, 2013-01-15 21:13:27]

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#newc...
base/safe_numerics.h:57: }  // namespace base
On 2013/01/15 21:09:50, brettw wrote:
> We've done some extern templates in some cases which GCC supports to avoid
> instantiating these in every module and slowing down the compile (see
logging.h)
> I wonder if it's worthwhile to define some of these for the several types that
> will cover 99% of the use cases. Don't feel like you need to do it in this
> patch.

fwiw, I was planning on just adding a saturating_cast, because it's shorter.

[brettw, 2013-01-15 21:15:47]

saturating_cast SGTM, although I still wonder if we need "checked" since the
current name doesn't actually describe what this function does (whereas
"saturating_cast"'s name does).

[scottmg, 2013-01-15 21:47:06]

On 2013/01/15 21:09:50, brettw wrote:
> Wondering out loud: what about calling this something like
checked_numeric_cast

sgtm. the name is based on boost's (which throws) but it isn't widely used
enough to warrant consistency.

> instead? One could imagine a saturating_numeric_cast or something for other
use
> cases.
> 
> 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#newc...
> base/safe_numerics.h:57: }  // namespace base
> We've done some extern templates in some cases which GCC supports to avoid
> instantiating these in every module and slowing down the compile (see
logging.h)
> I wonder if it's worthwhile to define some of these for the several types that
> will cover 99% of the use cases. Don't feel like you need to do it in this
> patch.

[scottmg, 2013-01-15 21:50:31]

On 2013/01/15 21:47:06, scottmg wrote:
> On 2013/01/15 21:09:50, brettw wrote:
> > Wondering out loud: what about calling this something like
> checked_numeric_cast
> 

Done.

[darin (slow to review), 2013-01-15 22:09:10]

LGTM

+1 to Brett's suggestion of instantiating common templates.

[cpu_(ooo_6.6-7.5), 2013-01-15 23:03:47]

lgtm

[brettw, 2013-01-16 00:11:46]

LGTM

[scottmg, 2013-01-16 16:19:49]

Unfortunately, the patch that everyone reviewed (patchset 5) only worked on VS
and gcc. clang was insistent that there was pointless code when it was
instantiated with an unsigned type:

../../base/safe_numerics.h:32:44:error: comparison of unsigned expression >= 0
is always true [-Werror,-Wtautological-compare]
    return DestLimits::is_signed || source >= 0;
                                    ~~~~~~ ^  ~

My apologies for not being more thorough before review.

I wasn't able to come up with a similar version that would avoid the warning.

Also, in revisiting, I discovered another problem. numeric_limits<>::digits is
the mantissa digits, and so numeric_limits<int>::digits is 31, whereas
numeric_limits<unsigned int>::digits is 32. So, the previous version was
probably wrong (or at least not doing what the comments said, and doing extra
compares).

Patchset 12 uses template specialization to accomplish the same job. I'm sure
you'll recoil in horror when you first see it. If you can get past the noise,
it's not that complicated (? Or maybe I've stared at it too long now).

There's 12 different specializations of the helper template that correspond to
the permutations of { SameSize, DestLarger, SourceLarger } x the combinations of
signed and unsigned. Each of these only checks the conditions required for that
specialization.

Then, the top level check passes in the size and sign information available at
compile time, and evaluates the associated condition.

If you could steel yourself for another review, it would be appreciated.

[jschuh, 2013-01-16 17:08:14]

On 2013/01/16 16:19:49, scottmg wrote:
> Unfortunately, the patch that everyone reviewed (patchset 5) only worked on VS
> and gcc. clang was insistent that there was pointless code when it was
> instantiated with an unsigned type:
> 
> ../../base/safe_numerics.h:32:44:error: comparison of unsigned expression >= 0
> is always true [-Werror,-Wtautological-compare]
>     return DestLimits::is_signed || source >= 0;
>                                     ~~~~~~ ^  ~
> 
> My apologies for not being more thorough before review.
> 
> I wasn't able to come up with a similar version that would avoid the warning.
> 
> Also, in revisiting, I discovered another problem. numeric_limits<>::digits is
> the mantissa digits, and so numeric_limits<int>::digits is 31, whereas
> numeric_limits<unsigned int>::digits is 32. So, the previous version was
> probably wrong (or at least not doing what the comments said, and doing extra
> compares).

Sorry, I should have put that in a comment. I was aware that digits are the
mantissa, but added the extra signedness compares in the first condition for
clarity, since they evaluate out as compile time constants.



> Patchset 12 uses template specialization to accomplish the same job. I'm sure
> you'll recoil in horror when you first see it. If you can get past the noise,
> it's not that complicated (? Or maybe I've stared at it too long now).
> 
> There's 12 different specializations of the helper template that correspond to
> the permutations of { SameSize, DestLarger, SourceLarger } x the combinations
of
> signed and unsigned. Each of these only checks the conditions required for
that
> specialization.
> 
> Then, the top level check passes in the size and sign information available at
> compile time, and evaluates the associated condition.
> 
> If you could steel yourself for another review, it would be appreciated.

[darin (slow to review), 2013-01-16 18:29:08]

OK, I yield.  LGTM

[commit-bot: I haz the power, 2013-01-16 20:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11886037/20

[commit-bot: I haz the power, 2013-01-16 23:31:03]

Retried try job too often on win7_aura for step(s) content_browsertests

[wtc, 2013-01-25 15:40:59]

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.

[scottmg, 2013-01-25 16:10:47]

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/