Add support for safe math operations in base/numerics

base/numerics/safe_conversions_impl.h

Index: base/numerics/safe_conversions_impl.h
diff --git a/base/numerics/safe_conversions_impl.h b/base/numerics/safe_conversions_impl.h
index 8bc30214f188abb48c72d6633506156af403863f..6e6609ebcb6bfb7e951ac04234fce509f4c5d4f5 100644
--- a/base/numerics/safe_conversions_impl.h
+++ b/base/numerics/safe_conversions_impl.h
@@ -8,171 +8,154 @@
 #include <limits>
 
 #include "base/macros.h"
+#include "base/template_util.h"
 
 namespace base {
 namespace internal {
 
-enum DstSign {
-  DST_UNSIGNED,
-  DST_SIGNED
-};
+using std::numeric_limits;
 
-enum SrcSign {
-  SRC_UNSIGNED,
-  SRC_SIGNED
-};
+enum DstSignId { DST_UNSIGNED = 0, DST_SIGNED = 1 };

[awong, 2014/02/11 19:40:10]

(push back on this if you disagree)

DstSignId and SrcSignId seems like it should be collapsed into one:

enum IntegerRepresentation {
  INTEGER_REPRESENTATION_SIGNED,
  INTEGER_REPRESENTATION_UNSIGNED,
};

or some sensible shortening of that.

I don't see too much benefit to differnt types. DstRepr and SrcRepr look better
as type-variable names rather than type names.

[jschuh, 2014/02/21 19:22:28]

Done. Fred actually wanted these split, because he thought it made the
specializations easier to follow. Although, collapsing them allows me to
eliminate one specialization (which I prefer), so I've collapsed them again and
will rely on the comments for additional clarity.

+
+enum SrcSignId { SRC_UNSIGNED = 0, SRC_SIGNED = 1 };
 
-enum DstRange {
-  OVERLAPS_RANGE,
-  CONTAINS_RANGE
+enum DstRangeId { OVERLAPS_RANGE = 0, CONTAINS_RANGE = 1 };

[awong, 2014/02/11 19:40:10]

nit:

enum DstRange {
  DST_RANGE_OVERLAPS,
  DST_RANGE_CONTAINS,
};

Also, can you add a comment explaining what exactly it means to be overlapping
and containing? Are we talking integers? floats? both? Open intervals? Closed?
Half-open?

Also, maybe move the enum closer to where you actually use it?

[jschuh, 2014/02/21 19:22:28]

Done.

+
+// Retrieve the max exponent for floating types and compute it for integrals.

[awong, 2014/02/11 19:40:10]

In the comment, prefer explaining why the math is sensible rather than what the
function is doing.

[jschuh, 2014/02/21 19:22:28]

Done.

+template <typename NumericType>

[awong, 2014/02/11 19:40:10]

s/NumericType/IntegralType/

Also, is there an easy COMPILE_ASSERT we can do to ensure this is an integral
type?  If not, don't worry...just curious.

[jschuh, 2014/02/21 19:22:28]

NumericType is correct because it handles both floating point and integral (but
floating point is just a pass-thru.
I could use an enable_if, if you want me to add that in later.

+struct MaxExponent {
+  static const int value = numeric_limits<NumericType>::is_iec559
+                               ? numeric_limits<NumericType>::max_exponent
+                               : (sizeof(NumericType) * 8 + 1 -
+                                  numeric_limits<NumericType>::is_signed);
 };
 
 // Helper templates to statically determine if our destination type can contain
-// all values represented by the source type.
+// maximum and minimum values represented by the source type.
 
-template <typename Dst, typename Src,
-          DstSign IsDstSigned = std::numeric_limits<Dst>::is_signed ?
-                                DST_SIGNED : DST_UNSIGNED,
-          SrcSign IsSrcSigned = std::numeric_limits<Src>::is_signed ?
-                                SRC_SIGNED : SRC_UNSIGNED>
-struct StaticRangeCheck {};
+template <typename Dst,
+          typename Src,
+          DstSignId DstSign = numeric_limits<Dst>::is_signed ? DST_SIGNED
+                                                             : DST_UNSIGNED,
+          SrcSignId SrcSign = numeric_limits<Src>::is_signed
+                                  ? SRC_SIGNED
+                                  : SRC_UNSIGNED > struct StaticRangeCheck {};

[awong, 2014/02/11 19:40:10]

Put the "struct" on a new line. As is, I spent 10 mins wondering how you managed
a "greater than" comparison with a type declaration.

Also, can you just make this a declaration instead of a struct definition?  I
don't see a reason to provide a default template. Only the specializations
provided below seem to make semantic sense.

[jschuh, 2014/02/21 19:22:28]

Done (but you can blame "git cl format" for that one :).

 
+// Both signed, narrowing.
 template <typename Dst, typename Src>
 struct StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_SIGNED> {
-  typedef std::numeric_limits<Dst> DstLimits;
-  typedef std::numeric_limits<Src> SrcLimits;
-  // Compare based on max_exponent, which we must compute for integrals.
-  static const size_t kDstMaxExponent = DstLimits::is_iec559 ?
-                                        DstLimits::max_exponent :
-                                        (sizeof(Dst) * 8 - 1);
-  static const size_t kSrcMaxExponent = SrcLimits::is_iec559 ?
-                                        SrcLimits::max_exponent :
-                                        (sizeof(Src) * 8 - 1);
-  static const DstRange value = kDstMaxExponent >= kSrcMaxExponent ?
-                                CONTAINS_RANGE : OVERLAPS_RANGE;
+  static const DstRangeId value =
+      MaxExponent<Dst>::value >= MaxExponent<Src>::value ? CONTAINS_RANGE
+                                                         : OVERLAPS_RANGE;
 };
 
+// Both unsigned, narrowing (handled same as both signed narrowing).
 template <typename Dst, typename Src>
-struct StaticRangeCheck<Dst, Src, DST_UNSIGNED, SRC_UNSIGNED> {
-  static const DstRange value = sizeof(Dst) >= sizeof(Src) ?
-                                CONTAINS_RANGE : OVERLAPS_RANGE;
-};
+struct StaticRangeCheck<
+    Dst,
+    Src,
+    DST_UNSIGNED,
+    SRC_UNSIGNED> : StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_SIGNED> {};
 
+// Unsigned to signed, overlapping.

[awong, 2014/02/11 19:40:10]

These comments in general don't explain why the relations are correct. Please
update here and below.

[jschuh, 2014/02/21 19:22:28]

Done.

 template <typename Dst, typename Src>
 struct StaticRangeCheck<Dst, Src, DST_SIGNED, SRC_UNSIGNED> {

[awong, 2014/02/11 19:40:10]

Maybe called this DstRangeRelationToSrcRange

or something that implies that you mean
  dst contains src, and not src contains dest.

[jschuh, 2014/02/21 19:22:28]

Done.

-  typedef std::numeric_limits<Dst> DstLimits;
-  typedef std::numeric_limits<Src> SrcLimits;
-  // Compare based on max_exponent, which we must compute for integrals.
-  static const size_t kDstMaxExponent = DstLimits::is_iec559 ?
-                                        DstLimits::max_exponent :
-                                        (sizeof(Dst) * 8 - 1);
-  static const size_t kSrcMaxExponent = sizeof(Src) * 8;
-  static const DstRange value = kDstMaxExponent >= kSrcMaxExponent ?
-                                CONTAINS_RANGE : OVERLAPS_RANGE;
+  static const DstRangeId value =
+      MaxExponent<Dst>::value > MaxExponent<Src>::value ? CONTAINS_RANGE

[jschuh, 2014/02/21 19:22:28]

Done.

+                                                        : OVERLAPS_RANGE;
 };
 
+// Signed to unsigned, overlapping.
 template <typename Dst, typename Src>
 struct StaticRangeCheck<Dst, Src, DST_UNSIGNED, SRC_SIGNED> {
-  static const DstRange value = OVERLAPS_RANGE;
+  static const DstRangeId value = OVERLAPS_RANGE;
 };
 
-
-enum RangeCheckResult {
+enum RangeCheckId {

[awong, 2014/02/11 19:40:10]

nit: "Id" is a weird suffix for enums here. These aren't really identifiers.  I
would call it something like RangeConstraints or something.

[jschuh, 2014/02/21 19:22:28]

Done.

   TYPE_VALID = 0,      // Value can be represented by the destination type.
   TYPE_UNDERFLOW = 1,  // Value would overflow.
   TYPE_OVERFLOW = 2,   // Value would underflow.
   TYPE_INVALID = 3     // Source value is invalid (i.e. NaN).
 };
 
-// This macro creates a RangeCheckResult from an upper and lower bound
+// This macro creates a RangeCheckId from an upper and lower bound
 // check by taking advantage of the fact that only NaN can be out of range in
 // both directions at once.
 #define BASE_NUMERIC_RANGE_CHECK_RESULT(is_in_upper_bound, is_in_lower_bound) \

[awong, 2014/02/11 19:40:10]

Why is this a macro instead of a static inline function?


static inline RangeCheckId GetRangeType(bool is_in_upper_bound, bool
is_in_lower_bound) {
   ((is_in_upper_bound) ? 0 : TYPE_OVERFLOW)
   ((is_in_lower_bound) ? 0 : TYPE_UNDERFLOW)
};

[jschuh, 2014/02/21 19:22:28]

Done. This needed to be a macro several versions back, but you're right that it
can just be an inline now.

-    RangeCheckResult(((is_in_upper_bound) ? 0 : TYPE_OVERFLOW) | \
-                            ((is_in_lower_bound) ? 0 : TYPE_UNDERFLOW))
+  RangeCheckId(((is_in_upper_bound) ? 0 : TYPE_OVERFLOW) |                    \
+               ((is_in_lower_bound) ? 0 : TYPE_UNDERFLOW))
 
 template <typename Dst,
           typename Src,
-          DstSign IsDstSigned = std::numeric_limits<Dst>::is_signed ?
-                                DST_SIGNED : DST_UNSIGNED,
-          SrcSign IsSrcSigned = std::numeric_limits<Src>::is_signed ?
-                                SRC_SIGNED : SRC_UNSIGNED,
-          DstRange IsSrcRangeContained = StaticRangeCheck<Dst, Src>::value>
-struct RangeCheckImpl {};
+          DstSignId DstSign = numeric_limits<Dst>::is_signed ? DST_SIGNED
+                                                             : DST_UNSIGNED,
+          SrcSignId SrcSign =
+              numeric_limits<Src>::is_signed ? SRC_SIGNED : SRC_UNSIGNED,
+          DstRangeId DstRange =
+              StaticRangeCheck<Dst, Src>::value > struct RangeCheckImpl {};

[awong, 2014/02/11 19:40:10]

Don't define this struct. Only declare it.

That way you get a whitelist of valid specializations.

Invoking the "default" specialization doesn't seem semantically valid.

[jschuh, 2014/02/21 19:22:28]

Done.

 
 // The following templates are for ranges that must be verified at runtime. We
 // split it into checks based on signedness to avoid confusing casts and
 // compiler warnings on signed an unsigned comparisons.
 
 // Dst range always contains the result: nothing to check.
-template <typename Dst, typename Src, DstSign IsDstSigned, SrcSign IsSrcSigned>
-struct RangeCheckImpl<Dst, Src, IsDstSigned, IsSrcSigned, CONTAINS_RANGE> {
-  static RangeCheckResult Check(Src value) {
-    return TYPE_VALID;
-  }
+template <typename Dst, typename Src, DstSignId DstSign, SrcSignId SrcSign>
+struct RangeCheckImpl<Dst, Src, DstSign, SrcSign, CONTAINS_RANGE> {
+  static RangeCheckId Check(Src value) { return TYPE_VALID; }
 };
 
 // Signed to signed narrowing.
 template <typename Dst, typename Src>
 struct RangeCheckImpl<Dst, Src, DST_SIGNED, SRC_SIGNED, OVERLAPS_RANGE> {
-  static RangeCheckResult Check(Src value) {
-    typedef std::numeric_limits<Dst> DstLimits;
-    return DstLimits::is_iec559 ?
-           BASE_NUMERIC_RANGE_CHECK_RESULT(
-               value <= static_cast<Src>(DstLimits::max()),
-               value >= static_cast<Src>(DstLimits::max() * -1)) :
-           BASE_NUMERIC_RANGE_CHECK_RESULT(
-               value <= static_cast<Src>(DstLimits::max()),
-               value >= static_cast<Src>(DstLimits::min()));
+  static RangeCheckId Check(Src value) {
+    return numeric_limits<Dst>::is_iec559
+               ? BASE_NUMERIC_RANGE_CHECK_RESULT(
+                     value <= numeric_limits<Dst>::max(),
+                     value >= -numeric_limits<Dst>::max())
+               : BASE_NUMERIC_RANGE_CHECK_RESULT(
+                     value <= numeric_limits<Dst>::max(),
+                     value >= numeric_limits<Dst>::min());
   }
 };
 
 // Unsigned to unsigned narrowing.
 template <typename Dst, typename Src>
 struct RangeCheckImpl<Dst, Src, DST_UNSIGNED, SRC_UNSIGNED, OVERLAPS_RANGE> {
-  static RangeCheckResult Check(Src value) {
-    typedef std::numeric_limits<Dst> DstLimits;
-    return BASE_NUMERIC_RANGE_CHECK_RESULT(
-               value <= static_cast<Src>(DstLimits::max()), true);
+  static RangeCheckId Check(Src value) {
+    return BASE_NUMERIC_RANGE_CHECK_RESULT(value <= numeric_limits<Dst>::max(),
+                                           true);
   }
 };
 
 // Unsigned to signed.
 template <typename Dst, typename Src>
 struct RangeCheckImpl<Dst, Src, DST_SIGNED, SRC_UNSIGNED, OVERLAPS_RANGE> {
-  static RangeCheckResult Check(Src value) {
-    typedef std::numeric_limits<Dst> DstLimits;
-    return sizeof(Dst) > sizeof(Src) ? TYPE_VALID :
-           BASE_NUMERIC_RANGE_CHECK_RESULT(
-               value <= static_cast<Src>(DstLimits::max()), true);
+  static RangeCheckId Check(Src value) {
+    return sizeof(Dst) > sizeof(Src)
+               ? TYPE_VALID
+               : BASE_NUMERIC_RANGE_CHECK_RESULT(
+                     value <= static_cast<Src>(numeric_limits<Dst>::max()),
+                     true);
   }
 };
 
 // Signed to unsigned.
 template <typename Dst, typename Src>
 struct RangeCheckImpl<Dst, Src, DST_UNSIGNED, SRC_SIGNED, OVERLAPS_RANGE> {
-  static RangeCheckResult Check(Src value) {
-    typedef std::numeric_limits<Dst> DstLimits;
-    typedef std::numeric_limits<Src> SrcLimits;
-    // Compare based on max_exponent, which we must compute for integrals.
-    static const size_t kDstMaxExponent = sizeof(Dst) * 8;
-    static const size_t kSrcMaxExponent = SrcLimits::is_iec559 ?
-                                          SrcLimits::max_exponent :
-                                          (sizeof(Src) * 8 - 1);
-    return (kDstMaxExponent >= kSrcMaxExponent) ?
-           BASE_NUMERIC_RANGE_CHECK_RESULT(true, value >= static_cast<Src>(0)) :
-           BASE_NUMERIC_RANGE_CHECK_RESULT(
-               value <= static_cast<Src>(DstLimits::max()),
-               value >= static_cast<Src>(0));
+  static RangeCheckId Check(Src value) {
+    return (MaxExponent<Dst>::value >= MaxExponent<Src>::value)
+               ? BASE_NUMERIC_RANGE_CHECK_RESULT(true,
+                                                 value >= static_cast<Src>(0))
+               : BASE_NUMERIC_RANGE_CHECK_RESULT(
+                     value <= static_cast<Src>(numeric_limits<Dst>::max()),
+                     value >= static_cast<Src>(0));
   }
 };
 
 template <typename Dst, typename Src>
-inline RangeCheckResult RangeCheck(Src value) {
-  COMPILE_ASSERT(std::numeric_limits<Src>::is_specialized,
-                 argument_must_be_numeric);
-  COMPILE_ASSERT(std::numeric_limits<Dst>::is_specialized,
-                 result_must_be_numeric);
+inline RangeCheckId RangeCheck(Src value) {
+  COMPILE_ASSERT(numeric_limits<Src>::is_specialized, argument_must_be_numeric);
+  COMPILE_ASSERT(numeric_limits<Dst>::is_specialized, result_must_be_numeric);
   return RangeCheckImpl<Dst, Src>::Check(value);
 }