Loosen restrictions on CheckedNumeric bitwise operators

base/numerics/safe_math_impl.h

Index: base/numerics/safe_math_impl.h
diff --git a/base/numerics/safe_math_impl.h b/base/numerics/safe_math_impl.h
index 68ad22af5a572c6a380597177b7104ce202a8842..15de619e76fae11ce35d8ecac5b950c4e7ba432b 100644
--- a/base/numerics/safe_math_impl.h
+++ b/base/numerics/safe_math_impl.h
@@ -602,19 +602,17 @@ struct CheckedRshOp<
 template <typename T, typename U, class Enable = void>
 struct CheckedAndOp {};
 
-// For simplicity we support only unsigned integers.
+// For simplicity we support only unsigned integer results.
 template <typename T, typename U>
 struct CheckedAndOp<T,
                     U,
                     typename std::enable_if<std::is_integral<T>::value &&
-                                            std::is_integral<U>::value &&
-                                            !std::is_signed<T>::value &&
-                                            !std::is_signed<U>::value>::type> {
-  using result_type =
-      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type;
+                                            std::is_integral<U>::value>::type> {
+  using result_type = typename UnsignedIntegerForSize<
+      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type>::type;

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

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]

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.

   template <typename V = result_type>
   static bool Do(T x, U y, V* result) {
-    result_type tmp = x & y;
+    result_type tmp = static_cast<result_type>(x) & static_cast<result_type>(y);
     *result = static_cast<V>(tmp);
     return IsValueInRangeForNumericType<V>(tmp);
   }
@@ -628,14 +626,12 @@ template <typename T, typename U>
 struct CheckedOrOp<T,
                    U,
                    typename std::enable_if<std::is_integral<T>::value &&
-                                           std::is_integral<U>::value &&
-                                           !std::is_signed<T>::value &&
-                                           !std::is_signed<U>::value>::type> {
-  using result_type =
-      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type;
+                                           std::is_integral<U>::value>::type> {
+  using result_type = typename UnsignedIntegerForSize<
+      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type>::type;
   template <typename V = result_type>
   static bool Do(T x, U y, V* result) {
-    result_type tmp = x | y;
+    result_type tmp = static_cast<result_type>(x) | static_cast<result_type>(y);
     *result = static_cast<V>(tmp);
     return IsValueInRangeForNumericType<V>(tmp);
   }
@@ -649,14 +645,12 @@ template <typename T, typename U>
 struct CheckedXorOp<T,
                     U,
                     typename std::enable_if<std::is_integral<T>::value &&
-                                            std::is_integral<U>::value &&
-                                            !std::is_signed<T>::value &&
-                                            !std::is_signed<U>::value>::type> {
-  using result_type =
-      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type;
+                                            std::is_integral<U>::value>::type> {
+  using result_type = typename UnsignedIntegerForSize<
+      typename ArithmeticPromotion<MAX_EXPONENT_PROMOTION, T, U>::type>::type;
   template <typename V = result_type>
   static bool Do(T x, U y, V* result) {
-    result_type tmp = x ^ y;
+    result_type tmp = static_cast<result_type>(x) ^ static_cast<result_type>(y);
     *result = static_cast<V>(tmp);
     return IsValueInRangeForNumericType<V>(tmp);
   }