Cleanup RangeCheck class and dependencies

Moves the test code out of RangeCheck. Reorders range comparisons and
removes bitfields to avoid a GCC constexpr bug. Removes some custom
optimizations that are no longer necessary after the restructuring.

TBR=tsepez

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

base/numerics/safe_conversions.h

@@ -106,39 +106,7 @@ struct SaturationDefaultHandler {
 
 namespace internal {
 
-template <typename T, template <typename> class S>
-struct IsDefaultIntegralBounds {
-  static const bool value =
-      std::is_integral<T>::value &&
-      S<T>::max() == SaturationDefaultHandler<T>::max() &&
-      S<T>::lowest() == SaturationDefaultHandler<T>::lowest();
-};
-
-// Integral to integral conversions have a special optimization for the
-// standard bounds.
-template <typename Dst,
-          template <typename> class S,
-          typename Src,
-          typename std::enable_if<
-              std::is_integral<Src>::value &&
-              IsDefaultIntegralBounds<Dst, S>::value>::type* = nullptr>
-constexpr Dst saturated_cast_impl(Src value, RangeCheck constraint) {
-  using UnsignedDst = typename std::make_unsigned<Dst>::type;
-  // The member fields in this class are lined up such that the compiler
-  // can saturate without branching in this case by adding the register
-  // with the bitfields directly to the integral max.
-  return constraint.IsValid()
-             ? static_cast<Dst>(value)
-             : static_cast<Dst>(UnsignedDst(constraint.IsUnderflowFlagSet()) +
-                                std::numeric_limits<Dst>::max());
-}
-
-template <typename Dst,
-          template <typename> class S,
-          typename Src,
-          typename std::enable_if<
-              !std::is_integral<Src>::value ||
-              !IsDefaultIntegralBounds<Dst, S>::value>::type* = nullptr>
+template <typename Dst, template <typename> class S, typename Src>
 constexpr Dst saturated_cast_impl(Src value, RangeCheck constraint) {
   // For some reason clang generates much better code when the branch is
   // structured exactly this way, rather than a sequence of checks.

base/numerics/safe_conversions_impl.h

@@ -136,18 +136,11 @@ struct StaticDstRangeRelationToSrcRange<Dst,
   static const NumericRangeRepresentation value = NUMERIC_RANGE_NOT_CONTAINED;
 };
 
-enum RangeConstraint {
-  RANGE_VALID = 0x0,      // Value can be represented by the destination type.
-  RANGE_UNDERFLOW = 0x1,  // Value would underflow.
-  RANGE_OVERFLOW = 0x2,   // Value would overflow.
-  RANGE_INVALID = RANGE_UNDERFLOW | RANGE_OVERFLOW  // Invalid (i.e. NaN).
-};
-
 // This class wraps the range constraints as separate booleans so the compiler
 // can identify constants and eliminate unused code paths.
 class RangeCheck {
  public:
-  constexpr RangeCheck(bool is_in_upper_bound, bool is_in_lower_bound)
+  constexpr RangeCheck(bool is_in_lower_bound, bool is_in_upper_bound)
       : is_underflow_(!is_in_lower_bound), is_overflow_(!is_in_upper_bound) {}
   constexpr RangeCheck() : is_underflow_(0), is_overflow_(0) {}
   constexpr bool IsValid() const { return !is_overflow_ && !is_underflow_; }
@@ -156,21 +149,19 @@ class RangeCheck {
   constexpr bool IsUnderflow() const { return !is_overflow_ && is_underflow_; }
   constexpr bool IsOverflowFlagSet() const { return is_overflow_; }
   constexpr bool IsUnderflowFlagSet() const { return is_underflow_; }
-
-  // These are some wrappers to make the tests a bit cleaner.
-  constexpr operator RangeConstraint() const {
-    return static_cast<RangeConstraint>(static_cast<int>(is_overflow_) << 1 |
-                                        static_cast<int>(is_underflow_));
+  constexpr bool operator==(const RangeCheck rhs) const {
+    return is_underflow_ == rhs.is_underflow_ &&
+           is_overflow_ == rhs.is_overflow_;
   }
-  constexpr bool operator==(const RangeConstraint rhs) const {
-    return rhs == static_cast<RangeConstraint>(*this);
+  constexpr bool operator!=(const RangeCheck rhs) const {
+    return !(*this == rhs);
   }
 
  private:
   // Do not change the order of these member variables. The integral conversion
   // optimization depends on this exact order.
-  const bool is_underflow_ : 1;
-  const bool is_overflow_ : 1;
+  const bool is_underflow_;
+  const bool is_overflow_;
 };
 
 // The following helper template addresses a corner case in range checks for
@@ -265,10 +256,10 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
     using SrcLimits = std::numeric_limits<Src>;
     using DstLimits = NarrowingRange<Dst, Src, Bounds>;
     return RangeCheck(
-        static_cast<Dst>(SrcLimits::max()) <= DstLimits::max() ||
-            static_cast<Dst>(value) <= DstLimits::max(),
         static_cast<Dst>(SrcLimits::lowest()) >= DstLimits::lowest() ||
-            static_cast<Dst>(value) >= DstLimits::lowest());
+            static_cast<Dst>(value) >= DstLimits::lowest(),
+        static_cast<Dst>(SrcLimits::max()) <= DstLimits::max() ||
+            static_cast<Dst>(value) <= DstLimits::max());
   }
 };
 
@@ -283,7 +274,7 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
                                       NUMERIC_RANGE_NOT_CONTAINED> {
   static constexpr RangeCheck Check(Src value) {
     using DstLimits = NarrowingRange<Dst, Src, Bounds>;
-    return RangeCheck(value <= DstLimits::max(), value >= DstLimits::lowest());
+    return RangeCheck(value >= DstLimits::lowest(), value <= DstLimits::max());
   }
 };
 
@@ -299,8 +290,8 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
   static constexpr RangeCheck Check(Src value) {
     using DstLimits = NarrowingRange<Dst, Src, Bounds>;
     return RangeCheck(
-        value <= DstLimits::max(),
-        DstLimits::lowest() == Dst(0) || value >= DstLimits::lowest());
+        DstLimits::lowest() == Dst(0) || value >= DstLimits::lowest(),
+        value <= DstLimits::max());
   }
 };
 
@@ -315,11 +306,11 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
   static constexpr RangeCheck Check(Src value) {
     using DstLimits = NarrowingRange<Dst, Src, Bounds>;
     using Promotion = decltype(Src() + Dst());
-    return RangeCheck(static_cast<Promotion>(value) <=
-                          static_cast<Promotion>(DstLimits::max()),
-                      DstLimits::lowest() <= Dst(0) ||
+    return RangeCheck(DstLimits::lowest() <= Dst(0) ||
                           static_cast<Promotion>(value) >=
-                              static_cast<Promotion>(DstLimits::lowest()));
+                              static_cast<Promotion>(DstLimits::lowest()),
+                      static_cast<Promotion>(value) <=
+                          static_cast<Promotion>(DstLimits::max()));
   }
 };
 
@@ -337,12 +328,12 @@ struct DstRangeRelationToSrcRangeImpl<Dst,
     using DstLimits = NarrowingRange<Dst, Src, Bounds>;
     using Promotion = decltype(Src() + Dst());
     return RangeCheck(
+        value >= Src(0) && (DstLimits::lowest() == 0 ||
+                            static_cast<Dst>(value) >= DstLimits::lowest()),
         static_cast<Promotion>(SrcLimits::max()) <=
                 static_cast<Promotion>(DstLimits::max()) ||
             static_cast<Promotion>(value) <=
-                static_cast<Promotion>(DstLimits::max()),
-        value >= Src(0) && (DstLimits::lowest() == 0 ||
-                            static_cast<Dst>(value) >= DstLimits::lowest()));
+                static_cast<Promotion>(DstLimits::max()));
   }
 };
 
@@ -615,7 +606,7 @@ constexpr bool IsLessImpl(const L lhs,
                           const R rhs,
                           const RangeCheck l_range,
                           const RangeCheck r_range) {
-  return l_range == RANGE_UNDERFLOW || r_range == RANGE_OVERFLOW ||
+  return l_range.IsUnderflow() || r_range.IsOverflow() ||
          (l_range == r_range &&
           static_cast<decltype(lhs + rhs)>(lhs) <
               static_cast<decltype(lhs + rhs)>(rhs));
@@ -636,7 +627,7 @@ constexpr bool IsLessOrEqualImpl(const L lhs,
                                  const R rhs,
                                  const RangeCheck l_range,
                                  const RangeCheck r_range) {
-  return l_range == RANGE_UNDERFLOW || r_range == RANGE_OVERFLOW ||
+  return l_range.IsUnderflow() || r_range.IsOverflow() ||
          (l_range == r_range &&
           static_cast<decltype(lhs + rhs)>(lhs) <=
               static_cast<decltype(lhs + rhs)>(rhs));
@@ -657,7 +648,7 @@ constexpr bool IsGreaterImpl(const L lhs,
                              const R rhs,
                              const RangeCheck l_range,
                              const RangeCheck r_range) {
-  return l_range == RANGE_OVERFLOW || r_range == RANGE_UNDERFLOW ||
+  return l_range.IsOverflow() || r_range.IsUnderflow() ||
          (l_range == r_range &&
           static_cast<decltype(lhs + rhs)>(lhs) >
               static_cast<decltype(lhs + rhs)>(rhs));
@@ -678,7 +669,7 @@ constexpr bool IsGreaterOrEqualImpl(const L lhs,
                                     const R rhs,
                                     const RangeCheck l_range,
                                     const RangeCheck r_range) {
-  return l_range == RANGE_OVERFLOW || r_range == RANGE_UNDERFLOW ||
+  return l_range.IsOverflow() || r_range.IsUnderflow() ||
          (l_range == r_range &&
           static_cast<decltype(lhs + rhs)>(lhs) >=
               static_cast<decltype(lhs + rhs)>(rhs));

base/numerics/safe_numerics_unittest.cc

@@ -45,10 +45,7 @@ using base::saturated_cast;
 using base::strict_cast;
 using base::internal::MaxExponent;
 using base::internal::IntegerBitsPlusSign;
-using base::internal::RANGE_VALID;
-using base::internal::RANGE_INVALID;
-using base::internal::RANGE_OVERFLOW;
-using base::internal::RANGE_UNDERFLOW;
+using base::internal::RangeCheck;
 
 // These tests deliberately cause arithmetic boundary errors. If the compiler is
 // aggressive enough, it can const detect these errors, so we disable warnings.
@@ -516,10 +513,26 @@ enum NumericConversionType {
 template <typename Dst, typename Src, NumericConversionType conversion>
 struct TestNumericConversion {};
 
+enum RangeConstraint {
+  RANGE_VALID = 0x0,      // Value can be represented by the destination type.
+  RANGE_UNDERFLOW = 0x1,  // Value would underflow.
+  RANGE_OVERFLOW = 0x2,   // Value would overflow.
+  RANGE_INVALID = RANGE_UNDERFLOW | RANGE_OVERFLOW  // Invalid (i.e. NaN).
+};
+
+// These are some wrappers to make the tests a bit cleaner.
+constexpr RangeConstraint RangeCheckToEnum(const RangeCheck constraint) {
+  return static_cast<RangeConstraint>(
+      static_cast<int>(constraint.IsOverflowFlagSet()) << 1 |
+      static_cast<int>(constraint.IsUnderflowFlagSet()));
+}
+
 // EXPECT_EQ wrappers providing specific detail on test failures.
-#define TEST_EXPECTED_RANGE(expected, actual)                                  \
-  EXPECT_EQ(expected, base::internal::DstRangeRelationToSrcRange<Dst>(actual)) \
-      << "Conversion test: " << src << " value " << actual << " to " << dst    \
+#define TEST_EXPECTED_RANGE(expected, actual)                               \
+  EXPECT_EQ(expected,                                                       \
+            RangeCheckToEnum(                                               \
+                base::internal::DstRangeRelationToSrcRange<Dst>(actual)))   \
+      << "Conversion test: " << src << " value " << actual << " to " << dst \
       << " on line " << line
 
 template <typename Dst, typename Src>