Cleanup some undefined floating point behavior in base/numerics

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 617 matching lines @@
 // Integrals require quite a bit of additional housekeeping to manage state.
 template <typename T>
 class CheckedNumericState<T, NUMERIC_INTEGER> {
  private:
   // is_valid_ precedes value_ because member intializers in the constructors
   // are evaluated in field order, and is_valid_ must be read when initializing
   // value_.
   bool is_valid_;
   T value_;
 
+  // Ensures that a type conversion does not trigger undefined behavior.
+  template <typename Src>
+  static constexpr T WellDefinedConversionOrZero(const Src value,
+                                                 const bool is_valid) {
+    using SrcType = typename internal::UnderlyingType<Src>::type;
+    return (std::is_integral<Src>::value ||

[Nico, 2016/12/06 14:03:37]

Nice!

+            StaticDstRangeRelationToSrcRange<T, SrcType>::value ==
+                NUMERIC_RANGE_CONTAINED ||

[Nico, 2016/12/06 14:03:38]

Isn't the is_integral check on Src stronger than this already? In this
specialization, we know that Dst is integral. If src is not integral (i.e.
float), this will never be true, right? And if Src is integral, the is_integral
checks for that already. So maybe this part can just be omitted?

[jschuh, 2016/12/06 15:07:55]

Weird. Somehow I posted this in an incomplete state, while I was splitting
between floating point and int.

+            is_valid)
+               ? static_cast<T>(value)
+               : static_cast<T>(0);
+  }
+
  public:
   template <typename Src, NumericRepresentation type>
   friend class CheckedNumericState;
 
   constexpr CheckedNumericState() : is_valid_(true), value_(0) {}
 
   template <typename Src>
   constexpr CheckedNumericState(Src value, bool is_valid)
       : is_valid_(is_valid && IsValueInRangeForNumericType<T>(value)),
-        value_(is_valid_ ? static_cast<T>(value) : 0) {
+        value_(WellDefinedConversionOrZero(value, is_valid_)) {
     static_assert(std::is_arithmetic<Src>::value, "Argument must be numeric.");
   }
 
   // Copy constructor.
   template <typename Src>
   constexpr CheckedNumericState(const CheckedNumericState<Src>& rhs)
       : is_valid_(rhs.IsValid()),
-        value_(is_valid_ ? static_cast<T>(rhs.value()) : 0) {}
+        value_(WellDefinedConversionOrZero(rhs.value(), is_valid_)) {}
 
   template <typename Src>
   constexpr explicit CheckedNumericState(Src value)
       : is_valid_(IsValueInRangeForNumericType<T>(value)),
-        value_(is_valid_ ? static_cast<T>(value) : 0) {}
+        value_(WellDefinedConversionOrZero(value, is_valid_)) {}
 
   constexpr bool is_valid() const { return is_valid_; }
   constexpr T value() const { return value_; }
 };
 
 // Floating points maintain their own validity, but need translation wrappers.
 template <typename T>
 class CheckedNumericState<T, NUMERIC_FLOATING> {
  private:
   T value_;
 
  public:
   template <typename Src, NumericRepresentation type>
   friend class CheckedNumericState;
 
   constexpr CheckedNumericState() : value_(0.0) {}
 
   template <typename Src>
   constexpr CheckedNumericState(Src value, bool is_valid)
       : value_((is_valid && IsValueInRangeForNumericType<T>(value))

[Nico, 2016/12/06 14:03:37]

Did you mean to replace these with saturating_cast?

[jschuh, 2016/12/06 17:11:13]

Not exactly, but you were totally correct that the patch was missing something.

                    ? static_cast<T>(value)
                    : std::numeric_limits<T>::quiet_NaN()) {}
 
   template <typename Src>
   constexpr explicit CheckedNumericState(Src value)
       : value_(static_cast<T>(value)) {}

[Nico, 2016/12/06 14:03:37]

ditto

 
   // Copy constructor.
   template <typename Src>
   constexpr CheckedNumericState(const CheckedNumericState<Src>& rhs)
       : value_(static_cast<T>(rhs.value())) {}

[Nico, 2016/12/06 14:03:38]

ditto

 
   constexpr bool is_valid() const {
     // Written this way because std::isfinite is not reliably constexpr.
     // TODO(jschuh): Fix this if the libraries ever get fixed.
     return value_ <= std::numeric_limits<T>::max() &&
            value_ >= std::numeric_limits<T>::lowest();
   }
   constexpr T value() const { return value_; }
 };
 
 template <template <typename, typename, typename> class M,
           typename L,
           typename R>
 struct MathWrapper {
   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_