Loosen restrictions on CheckedNumeric bitwise operators

base/numerics/safe_math_impl.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_NUMERICS_SAFE_MATH_IMPL_H_
 #define BASE_NUMERICS_SAFE_MATH_IMPL_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 584 matching lines @@
       *result = static_cast<V>(tmp);
       return IsValueInRangeForNumericType<V>(tmp);
     }
     return false;
   }
 };
 
 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);
   }
 };
 
 template <typename T, typename U, class Enable = void>
 struct CheckedOrOp {};
 
 // For simplicity we support only unsigned integers.
 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);
   }
 };
 
 template <typename T, typename U, class Enable = void>
 struct CheckedXorOp {};
 
 // For simplicity we support only unsigned integers.
 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);
   }
 };
 
 template <typename T>
 typename std::enable_if<std::numeric_limits<T>::is_integer &&
                             std::numeric_limits<T>::is_signed,
                         bool>::type
 CheckedNeg(T value, T* result) {
@@ skipping 241 matching lines @@
   using math = M<typename UnderlyingType<L>::type,
                  typename UnderlyingType<R>::type,
                  void>;
   using type = typename math::result_type;
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_NUMERICS_SAFE_MATH_IMPL_H_