Loosen restrictions on CheckedNumeric bitwise operators

Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true

Committed: https://crrev.com/b6737bb0f0b1677cb608c86bbccd0bceb3e9fbcf
Cr-Commit-Position: refs/heads/master@{#434859}

[jschuh, 2016-11-28 15:39:43]

Patchset #3 (id:40001) has been deleted

[jschuh, 2016-11-28 15:40:21]

Patchset #2 (id:20001) has been deleted

[jschuh, 2016-11-28 15:43:19]

jschuh@chromium.org changed reviewers:
+ scottmg@chromium.org

[jschuh, 2016-11-28 15:43:20]

ptal - This is a small tweak, because it turns out these operators are way too
painful to use if you don't support signed operands (stupid C default int
promotions).

[jschuh, 2016-11-28 15:45:14]

The CQ bit was checked by jschuh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 15:45:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-11-28 16:37:14]

https://codereview.chromium.org/2529413002/diff/60001/base/numerics/safe_math...
File base/numerics/safe_math_impl.h (right):

https://codereview.chromium.org/2529413002/diff/60001/base/numerics/safe_math...
base/numerics/safe_math_impl.h:612: typename
ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type>::type;
Sorry, I don't understand how this works. Can you explain how this won't compile
for `CheckedNumeric<Dst>(1) & -1`, I assume somehow in ArithmeticPromotion?
(Otherwise the static_cast to uint32_t is UB, right?)

[jschuh, 2016-11-28 17:54:31]

https://codereview.chromium.org/2529413002/diff/60001/base/numerics/safe_math...
File base/numerics/safe_math_impl.h (right):

https://codereview.chromium.org/2529413002/diff/60001/base/numerics/safe_math...
base/numerics/safe_math_impl.h:612: typename
ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type>::type;
On 2016/11/28 16:37:14, scottmg (slow on 25nov) wrote:
> Sorry, I don't understand how this works. Can you explain how this won't
compile
> for `CheckedNumeric<Dst>(1) & -1`, I assume somehow in ArithmeticPromotion?
> (Otherwise the static_cast to uint32_t is UB, right?)

It will accept -1 because signed to unsigned conversion is well defined by the
standard. The narrower type is extended to the width of the wider type (signed
is sign-extended, unsigned is zero-extended), and the result is then bitwise
converted to the wider, unsigned type.

I tried converting some code and realized people are relying on exactly that
behavior with patterns like this:

  res = a & ~3  // Mask out the lower three bits.

So, rather than force a bunch of ugly casts, I think we can safely make this
work by just promoting everything to unsigned.

I've added some more tests and updated the existing ones to better reflect
what's it's doing.

[scottmg, 2016-11-28 18:02:48]

lgtm

[jschuh, 2016-11-28 18:11:55]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2016-11-28 18:12:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 20:43:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 20:43:15]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jschuh, 2016-11-28 20:57:34]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2016-11-28 20:58:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 22:07:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 22:07:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
no build URL)

[jschuh, 2016-11-28 22:18:33]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2016-11-28 22:19:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 02:28:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 02:28:09]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, no
build URL)

[jschuh, 2016-11-29 02:34:31]

Description was changed from

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.
==========

to

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true
==========

[jschuh, 2016-11-29 02:34:39]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2016-11-29 02:35:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 03:07:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480386879072220,
"parent_rev": "001efce8c8ad13b4da941b47d9f4f681f9d8b112", "commit_rev":
"b1e894d09b6107aa6c0ee685f3046b10fe4e354a"}

[commit-bot: I haz the power, 2016-11-29 03:07:30]

Description was changed from

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true
==========

to

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true
==========

[commit-bot: I haz the power, 2016-11-29 03:07:31]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-11-29 03:11:13]

Description was changed from

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true
==========

to

==========
Loosen restrictions on CheckedNumeric bitwise operators

Allow any integer types but always promote the result to unsigned.
This is more consistent with how these operations are normally used.

NOTRY=true

Committed: https://crrev.com/b6737bb0f0b1677cb608c86bbccd0bceb3e9fbcf
Cr-Commit-Position: refs/heads/master@{#434859}
==========

[commit-bot: I haz the power, 2016-11-29 03:11:14]

Patchset 3 (id:??) landed as
https://crrev.com/b6737bb0f0b1677cb608c86bbccd0bceb3e9fbcf
Cr-Commit-Position: refs/heads/master@{#434859}