Revert of Support saturation overrides in saturated_cast (patchset #7...

Revert of Support saturation overrides in saturated_cast (patchset #7 id:180001 of https://codereview.chromium.org/2578613002/ )

Reason for revert:
Suspect breaking base_unittests (missing test name in unittest macro).
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/38202

Original issue's description:
> Support saturation overrides in saturated_cast
>
> This is a requirement for adding a full saturated type implementation.
>
> BUG=672489
> NOTRY=true
>
> Committed: https://crrev.com/186c7dbbea5eef852bf0c4277d1da16f56f9156e
> Cr-Commit-Position: refs/heads/master@{#439061}

TBR=eae@chromium.org,jschuh@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=672489,674849

Review-Url: https://codereview.chromium.org/2582063002
Cr-Commit-Position: refs/heads/master@{#439150}

base/numerics/safe_conversions.h

@@ -55,6 +55,22 @@ constexpr bool IsValueInRangeForNumericType(Src value) {
          internal::RANGE_VALID;
 }
 
+// Convenience function for determining if a numeric value is negative without
+// throwing compiler warnings on: unsigned(value) < 0.
+template <typename T,
+          typename std::enable_if<std::is_signed<T>::value>::type* = nullptr>
+constexpr bool IsValueNegative(T value) {
+  static_assert(std::is_arithmetic<T>::value, "Argument must be numeric.");
+  return value < 0;
+}
+
+template <typename T,
+          typename std::enable_if<!std::is_signed<T>::value>::type* = nullptr>
+constexpr bool IsValueNegative(T) {
+  static_assert(std::is_arithmetic<T>::value, "Argument must be numeric.");
+  return false;
+}
+
 // Forces a crash, like a CHECK(false). Used for numeric boundary errors.
 struct CheckOnFailure {
   template <typename T>
@@ -83,62 +99,62 @@ constexpr Dst checked_cast(Src value) {
              : CheckHandler::template HandleFailure<Dst>();
 }
 
-// Default boundaries for integral/float: max/infinity, lowest/-infinity, 0/NaN.
-template <typename T>
-struct SaturatedCastDefaultHandler {
-  static constexpr T HandleNaN() {
-    return std::numeric_limits<T>::has_quiet_NaN
-               ? std::numeric_limits<T>::quiet_NaN()
-               : T();
-  }
-  static constexpr T max() { return std::numeric_limits<T>::max(); }
-  static constexpr T HandleOverflow() {
-    return std::numeric_limits<T>::has_infinity
-               ? std::numeric_limits<T>::infinity()
-               : std::numeric_limits<T>::max();
-  }
-  static constexpr T lowest() { return std::numeric_limits<T>::lowest(); }
-  static constexpr T HandleUnderflow() {
-    return std::numeric_limits<T>::has_infinity
-               ? std::numeric_limits<T>::infinity() * -1
-               : std::numeric_limits<T>::lowest();
+// HandleNaN will return 0 in this case.
+struct SaturatedCastNaNBehaviorReturnZero {
+  template <typename T>
+  static constexpr T HandleFailure() {
+    return T();
   }
 };
 
 namespace internal {
+// These wrappers are used for C++11 constexpr support by avoiding both the
+// declaration of local variables and invalid evaluation resulting from the
+// lack of "constexpr if" support in the saturated_cast template function.
+// TODO(jschuh): Convert to single function with a switch once we support C++14.
+template <
+    typename Dst,
+    class NaNHandler,
+    typename Src,
+    typename std::enable_if<std::is_integral<Dst>::value>::type* = nullptr>
+constexpr Dst saturated_cast_impl(const Src value,
+                                  const RangeConstraint constraint) {
+  return constraint == RANGE_VALID
+             ? static_cast<Dst>(value)
+             : (constraint == RANGE_UNDERFLOW
+                    ? std::numeric_limits<Dst>::lowest()
+                    : (constraint == RANGE_OVERFLOW
+                           ? std::numeric_limits<Dst>::max()
+                           : NaNHandler::template HandleFailure<Dst>()));
+}
+
+template <typename Dst,
+          class NaNHandler,
+          typename Src,
+          typename std::enable_if<std::is_floating_point<Dst>::value>::type* =
+              nullptr>
+constexpr Dst saturated_cast_impl(const Src value,
+                                  const RangeConstraint constraint) {
+  return constraint == RANGE_VALID
+             ? static_cast<Dst>(value)
+             : (constraint == RANGE_UNDERFLOW
+                    ? -std::numeric_limits<Dst>::infinity()
+                    : (constraint == RANGE_OVERFLOW
+                           ? std::numeric_limits<Dst>::infinity()
+                           : std::numeric_limits<Dst>::quiet_NaN()));
+}
+
 // saturated_cast<> is analogous to static_cast<> for numeric types, except
-// that the specified numeric conversion will saturate by default rather than
-// overflow or underflow, and NaN assignment to an integral will return 0.
-// All boundary condition behaviors can be overriden with a custom handler.
+// that the specified numeric conversion will saturate rather than overflow or
+// underflow. NaN assignment to an integral will defer the behavior to a
+// specified class. By default, it will return 0.
 template <typename Dst,
-          template <typename>
-          class SaturationHandler = SaturatedCastDefaultHandler,
+          class NaNHandler = SaturatedCastNaNBehaviorReturnZero,
           typename Src>
 constexpr Dst saturated_cast(Src value) {
-  static_assert(
-      SaturationHandler<Dst>::lowest() < SaturationHandler<Dst>::max(), "");
-  // While this looks like a lot of code, it's all constexpr and all but
-  // one variable are compile-time constants (enforced by a static_assert).
-  // So, it should evaluate to the minimum number of comparisons required
-  // for the range check, which is 0-3, depending on the exact source and
-  // destination types, and whatever custom range is specified.
   using SrcType = typename UnderlyingType<Src>::type;
-  return IsGreaterOrEqual<SrcType, Dst>::Test(
-             value, NarrowingRange<Dst, SrcType, SaturationHandler>::lowest())
-             ? (IsLessOrEqual<SrcType, Dst>::Test(
-                    value,
-                    NarrowingRange<Dst, SrcType, SaturationHandler>::max())
-                    ? static_cast<Dst>(value)
-                    : SaturationHandler<Dst>::HandleOverflow())
-             // This last branch is a little confusing. It's specifically to
-             // catch NaN when converting from float to integral.
-             : (std::is_integral<SrcType>::value ||
-                        std::is_floating_point<Dst>::value ||
-                        IsLessOrEqual<SrcType, Dst>::Test(
-                            value, NarrowingRange<Dst, SrcType,
-                                                  SaturationHandler>::max())
-                    ? SaturationHandler<Dst>::HandleUnderflow()
-                    : SaturationHandler<Dst>::HandleNaN());
+  return internal::saturated_cast_impl<Dst, NaNHandler>(
+      value, internal::DstRangeRelationToSrcRange<Dst, SrcType>(value));
 }
 
 // strict_cast<> is analogous to static_cast<> for numeric types, except that
@@ -269,7 +285,6 @@ using internal::saturated_cast;
 using internal::SafeUnsignedAbs;
 using internal::StrictNumeric;
 using internal::MakeStrictNum;
-using internal::IsValueNegative;
 
 // Explicitly make a shorter size_t alias for convenience.
 using SizeT = StrictNumeric<size_t>;

base/numerics/safe_conversions_impl.h

@@ -50,36 +50,6 @@ constexpr T BinaryComplement(T x) {
   return static_cast<T>(~x);
 }
 
-// Determines if a numeric value is negative without throwing compiler
-// warnings on: unsigned(value) < 0.
-template <typename T,
-          typename std::enable_if<std::is_signed<T>::value>::type* = nullptr>
-constexpr bool IsValueNegative(T value) {
-  static_assert(std::is_arithmetic<T>::value, "Argument must be numeric.");
-  return value < 0;
-}
-
-template <typename T,
-          typename std::enable_if<!std::is_signed<T>::value>::type* = nullptr>
-constexpr bool IsValueNegative(T) {
-  static_assert(std::is_arithmetic<T>::value, "Argument must be numeric.");
-  return false;
-}
-
-// This performs a fast negation, returning a signed value. It works on unsigned
-// arguments, but probably doesn't do what you want for any unsigned value
-// larger than max / 2 + 1 (i.e. signed min cast to unsigned).
-template <typename T>
-constexpr typename std::make_signed<T>::type ConditionalNegate(
-    T x,
-    bool is_negative) {
-  static_assert(std::is_integral<T>::value, "Type must be integral");
-  using SignedT = typename std::make_signed<T>::type;
-  using UnsignedT = typename std::make_unsigned<T>::type;
-  return static_cast<SignedT>(
-      (static_cast<UnsignedT>(x) ^ -SignedT(is_negative)) + is_negative);
-}
-
 // This performs a safe, non-branching absolute value via unsigned overflow.
 template <typename T>
 constexpr T SafeUnsignedAbsImpl(T value, T sign_mask) {
@@ -211,44 +181,28 @@ constexpr inline RangeConstraint GetRangeConstraint(bool is_in_upper_bound,
 // To fix this bug we manually truncate the maximum value when the destination
 // type is an integral of larger precision than the source floating-point type,
 // such that the resulting maximum is represented exactly as a floating point.
-template <typename Dst,
-          typename Src,
-          template <typename> class Bounds = std::numeric_limits>
+template <typename Dst, typename Src>
 struct NarrowingRange {
   using SrcLimits = typename std::numeric_limits<Src>;
   using DstLimits = typename std::numeric_limits<Dst>;
-
-  // Computes the mask required to make an accurate comparison between types.
-  static const int kShift =
-      (MaxExponent<Src>::value > MaxExponent<Dst>::value &&
-       SrcLimits::digits < DstLimits::digits)
+  // The following logic avoids warnings where the max function is
+  // instantiated with invalid values for a bit shift (even though
+  // such a function can never be called).
+  static const int shift = (MaxExponent<Src>::value > MaxExponent<Dst>::value &&
+                            SrcLimits::digits < DstLimits::digits &&
+                            SrcLimits::is_iec559 &&
+                            DstLimits::is_integer)
                                ? (DstLimits::digits - SrcLimits::digits)
                                : 0;
-  template <
-      typename T,
-      typename std::enable_if<std::is_integral<T>::value>::type* = nullptr>
-
-  // Masks out the integer bits that are beyond the precision of the
-  // intermediate type used for comparison.
-  static constexpr T Adjust(T value) {
-    static_assert(std::is_same<T, Dst>::value, "");
-    static_assert(kShift < DstLimits::digits, "");
-    return static_cast<T>(
-        ConditionalNegate(SafeUnsignedAbs(value) & ~((T(1) << kShift) - T(1)),
-                          IsValueNegative(value)));
-  }
 
-  template <typename T,
-            typename std::enable_if<std::is_floating_point<T>::value>::type* =
-                nullptr>
-  static constexpr T Adjust(T value) {
-    static_assert(std::is_same<T, Dst>::value, "");
-    static_assert(kShift == 0, "");
-    return value;
+  static constexpr Dst max() {
+    // We use UINTMAX_C below to avoid compiler warnings about shifting floating
+    // points. Since it's a compile time calculation, it shouldn't have any
+    // performance impact.
+    return DstLimits::max() - static_cast<Dst>((UINTMAX_C(1) << shift) - 1);
   }
 
-  static constexpr Dst max() { return Adjust(Bounds<Dst>::max()); }
-  static constexpr Dst lowest() { return Adjust(Bounds<Dst>::lowest()); }
+  static constexpr Dst lowest() { return DstLimits::lowest(); }
 };
 
 template <typename Dst,

base/numerics/safe_numerics_unittest.cc

@@ -831,25 +831,7 @@ static_assert(std::is_same<decltype(TestOverload(StrictNumeric<size_t>())),
                            size_t>::value,
               "");
 
-template <typename T>
-struct CastTest1 {
-  static constexpr T HandleNaN() { return -1; }
-  static constexpr T max() { return numeric_limits<T>::max() - 1; }
-  static constexpr T HandleOverflow() { return max(); }
-  static constexpr T lowest() { return numeric_limits<T>::lowest() + 1; }
-  static constexpr T HandleUnderflow() { return lowest(); }
-};
-
-template <typename T>
-struct CastTest2 {
-  static constexpr T HandleNaN() { return 11; }
-  static constexpr T max() { return 10; }
-  static constexpr T HandleOverflow() { return max(); }
-  static constexpr T lowest() { return 1; }
-  static constexpr T HandleUnderflow() { return lowest(); }
-};
-
-TEST(SafeNumerics, ) {
+TEST(SafeNumerics, CastTests) {
 // MSVC catches and warns that we're forcing saturation in these tests.
 // Since that's intentional, we need to shut this warning off.
 #if defined(COMPILER_MSVC)
@@ -911,35 +893,6 @@ TEST(SafeNumerics, ) {
             saturated_cast<int>(double_small_int));
   EXPECT_EQ(numeric_limits<int>::max(), saturated_cast<int>(double_large_int));
 
-  // Test the saturated cast overrides.
-  using FloatLimits = numeric_limits<float>;
-  using IntLimits = numeric_limits<int>;
-  EXPECT_EQ(-1, (saturated_cast<int, CastTest1>(FloatLimits::quiet_NaN())));
-  EXPECT_EQ(CastTest1<int>::max(),
-            (saturated_cast<int, CastTest1>(FloatLimits::infinity())));
-  EXPECT_EQ(CastTest1<int>::max(),
-            (saturated_cast<int, CastTest1>(FloatLimits::max())));
-  EXPECT_EQ(CastTest1<int>::max(),
-            (saturated_cast<int, CastTest1>(float(IntLimits::max()))));
-  EXPECT_EQ(CastTest1<int>::lowest(),
-            (saturated_cast<int, CastTest1>(-FloatLimits::infinity())));
-  EXPECT_EQ(CastTest1<int>::lowest(),
-            (saturated_cast<int, CastTest1>(FloatLimits::lowest())));
-  EXPECT_EQ(0, (saturated_cast<int, CastTest1>(0.0)));
-  EXPECT_EQ(1, (saturated_cast<int, CastTest1>(1.0)));
-  EXPECT_EQ(-1, (saturated_cast<int, CastTest1>(-1.0)));
-  EXPECT_EQ(0, (saturated_cast<int, CastTest1>(0)));
-  EXPECT_EQ(1, (saturated_cast<int, CastTest1>(1)));
-  EXPECT_EQ(-1, (saturated_cast<int, CastTest1>(-1)));
-  EXPECT_EQ(CastTest1<int>::lowest(),
-            (saturated_cast<int, CastTest1>(float(IntLimits::lowest()))));
-  EXPECT_EQ(11, (saturated_cast<int, CastTest2>(FloatLimits::quiet_NaN())));
-  EXPECT_EQ(10, (saturated_cast<int, CastTest2>(FloatLimits::infinity())));
-  EXPECT_EQ(10, (saturated_cast<int, CastTest2>(FloatLimits::max())));
-  EXPECT_EQ(1, (saturated_cast<int, CastTest2>(-FloatLimits::infinity())));
-  EXPECT_EQ(1, (saturated_cast<int, CastTest2>(FloatLimits::lowest())));
-  EXPECT_EQ(1, (saturated_cast<int, CastTest2>(0U)));
-
   float not_a_number = std::numeric_limits<float>::infinity() -
                        std::numeric_limits<float>::infinity();
   EXPECT_TRUE(std::isnan(not_a_number));
@@ -984,6 +937,15 @@ TEST(SafeNumerics, ) {
   EXPECT_EQ(1, strict_cast<int>(StrictNumeric<int>(1)));
 }
 
+TEST(SafeNumerics, SaturatedCastChecks) {
+  float not_a_number = std::numeric_limits<float>::infinity() -
+                       std::numeric_limits<float>::infinity();
+  EXPECT_TRUE(std::isnan(not_a_number));
+  EXPECT_DEATH_IF_SUPPORTED(
+      (saturated_cast<int, base::CheckOnFailure>(not_a_number)),
+      "");
+}
+
 TEST(SafeNumerics, IsValueInRangeForNumericType) {
   EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(0));
   EXPECT_TRUE(IsValueInRangeForNumericType<uint32_t>(1));