Add ClampedNumeric templates

base/numerics/checked_math_impl.h

Index: base/numerics/checked_math_impl.h
diff --git a/base/numerics/checked_math_impl.h b/base/numerics/checked_math_impl.h
index 0492d1dbb5540e4f7a90a51bf4217a0a07388b27..203b286282b2c779cc13fa4292e529a8ae4e654e 100644
--- a/base/numerics/checked_math_impl.h
+++ b/base/numerics/checked_math_impl.h
@@ -182,17 +182,19 @@ struct CheckedMulOp<T,
       return !__builtin_mul_overflow(x, y, result);
 #endif
     using Promotion = typename FastIntegerArithmeticPromotion<T, U>::type;
+    // Verify the destination type can hold the result (always true for 0).
+    if (!(IsValueInRangeForNumericType<Promotion>(x) &&
+          IsValueInRangeForNumericType<Promotion>(y)) &&
+        x && y) {
+      return false;

[jschuh, 2017/06/26 20:26:50]

The only functional change here is supporting the case where either operand is
zero, which is hit by some of the ClampedNumeric uses.

+    }
     Promotion presult;
-    // Fail if either operand is out of range for the promoted type.
-    // TODO(jschuh): This could be made to work for a broader range of values.
-    bool is_valid = IsValueInRangeForNumericType<Promotion>(x) &&
-                    IsValueInRangeForNumericType<Promotion>(y);
-
+    bool is_valid = true;
     if (IsIntegerArithmeticSafe<Promotion, T, U>::value) {
       presult = static_cast<Promotion>(x) * static_cast<Promotion>(y);
     } else {
-      is_valid &= CheckedMulImpl(static_cast<Promotion>(x),
-                                 static_cast<Promotion>(y), &presult);
+      is_valid = CheckedMulImpl(static_cast<Promotion>(x),
+                                static_cast<Promotion>(y), &presult);
     }
     *result = static_cast<V>(presult);
     return is_valid && IsValueInRangeForNumericType<V>(presult);
@@ -242,7 +244,7 @@ struct CheckedDivOp<T,
 template <typename T>
 bool CheckedModImpl(T x, T y, T* result) {
   static_assert(std::is_integral<T>::value, "Type must be integral");
-  if (y > 0) {
+  if (y) {
     *result = static_cast<T>(x % y);
     return true;
   }
@@ -272,9 +274,6 @@ struct CheckedModOp<T,
 template <typename T, typename U, class Enable = void>
 struct CheckedLshOp {};
 
-// Left shift. Shifts less than 0 or greater than or equal to the number
-// of bits in the promoted type are undefined. Shifts of negative values
-// are undefined. Otherwise it is defined when the result fits.
 template <typename T, typename U>
 struct CheckedLshOp<T,
                     U,
@@ -283,27 +282,22 @@ struct CheckedLshOp<T,
   using result_type = T;
   template <typename V>
   static bool Do(T x, U shift, V* result) {
-    using ShiftType = typename std::make_unsigned<T>::type;
-    static const ShiftType kBitWidth = IntegerBitsPlusSign<T>::value;
-    const ShiftType real_shift = static_cast<ShiftType>(shift);
-    // Signed shift is not legal on negative values.
-    if (!IsValueNegative(x) && real_shift < kBitWidth) {
+    if (!IsValueNegative(x) &&
+        as_unsigned(shift) < std::numeric_limits<T>::digits) {
       // Just use a multiplication because it's easy.
       // TODO(jschuh): This could probably be made more efficient.
-      if (!std::is_signed<T>::value || real_shift != kBitWidth - 1)
-        return CheckedMulOp<T, T>::Do(x, static_cast<T>(1) << shift, result);
-      return !x;  // Special case zero for a full width signed shift.
+      return CheckedMulOp<T, T>::Do(x, T(1) << shift, result);
     }
-    return false;
+    // The standard defines only the result of the shift, rather than the
+    // values of source operands. So, shifting zero by any amount is legal.
+    // Otherwise, we overflowed or have an undefined operation.
+    return !x ? (*result = V(0)), true : false;

[jschuh, 2017/06/26 20:26:50]

Stumbled on a spec error in how I was handling zero, so I chose to clean it up
in this CL in order to match the ClampedNumeric behavior.

[dcheng, 2017/06/28 07:25:06]

Acknowledged--though for future CLs, it would be nice to split fixes like this
out into a separate CL (since it makes it easier to review any associated test
changes). Dependent CLs are really easy in Gerrit ;-)

That being said... here's what I found in the C++11 spec in [expr.shift]/1:
The behavior is undefined if the right operand is negative, or greater than or
equal to the length in bits of the promoted left operand.

I don't see any caveat here for 0?

[jschuh, 2017/06/28 12:32:37]

Weird. I'm looking at it now and you're right. Maybe I was looking at a working
draft that was later changed. I'll fix it and the associated tests.

   }
 };
 
 template <typename T, typename U, class Enable = void>
 struct CheckedRshOp {};
 
-// Right shift. Shifts less than 0 or greater than or equal to the number
-// of bits in the promoted type are undefined. Otherwise, it is always defined,
-// but a right shift of a negative value is implementation-dependent.
 template <typename T, typename U>
 struct CheckedRshOp<T,
                     U,
@@ -314,7 +308,7 @@ struct CheckedRshOp<T,
   static bool Do(T x, U shift, V* result) {
     // Use the type conversion push negative values out of range.
     using ShiftType = typename std::make_unsigned<T>::type;
-    if (static_cast<ShiftType>(shift) < IntegerBitsPlusSign<T>::value) {
+    if (!x || static_cast<ShiftType>(shift) < IntegerBitsPlusSign<T>::value) {

[jschuh, 2017/06/26 20:26:50]

Stumbled on a spec error in how I was handling zero, so I had to clean it up
because the ClampedNumeric implementation relies on this function.

[dcheng, 2017/06/28 07:25:06]

Ditto.

[jschuh, 2017/06/28 12:32:37]

Yup.

       T tmp = x >> shift;
       *result = static_cast<V>(tmp);
       return IsValueInRangeForNumericType<V>(tmp);