Bug 849666 - Make CheckedInt<T>::operator-() not depend on undefined behavior when negating minimum signed values, and add a test for this. Patch is something of a tag-team effort by bjacob and me. r=bjacob
authorJeff Walden <jwalden@mit.edu>
Mon, 11 Mar 2013 18:45:22 -0700
changeset 124515 e1cee7393c7381523c32283ce6888ebb7d8a9d4c
parent 124500 79b8e0a0bdb70761769aeba0d7f6d08dcc759b49
child 124516 36ec9c5b8da421d162a8f681139e831743f49293
push id24427
push useremorley@mozilla.com
push dateWed, 13 Mar 2013 12:28:55 +0000
treeherdermozilla-central@072b936973fe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbjacob
bugs849666
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 849666 - Make CheckedInt<T>::operator-() not depend on undefined behavior when negating minimum signed values, and add a test for this. Patch is something of a tag-team effort by bjacob and me. r=bjacob
mfbt/CheckedInt.h
mfbt/tests/TestCheckedInt.cpp
--- a/mfbt/CheckedInt.h
+++ b/mfbt/CheckedInt.h
@@ -35,16 +35,18 @@
 
 #include "mozilla/StandardInteger.h"
 
 #include <climits>
 #include <cstddef>
 
 namespace mozilla {
 
+template<typename T> class CheckedInt;
+
 namespace detail {
 
 /*
  * Step 1: manually record supported types
  *
  * What's nontrivial here is that there are different families of integer
  * types: basic integer types and stdint types. It is merrily undefined which
  * types from one family may be just typedefs for a type from another family.
@@ -449,33 +451,42 @@ template<typename T>
 inline bool
 IsDivValid(T x, T y)
 {
   // Keep in mind that in the signed case, min/-1 is invalid because abs(min)>max.
   return y != 0 &&
          !(IsSigned<T>::value && x == MinValue<T>::value && y == T(-1));
 }
 
-// This is just to shut up msvc warnings about negating unsigned ints.
-template<typename T, bool IsTSigned = IsSigned<T>::value>
-struct OppositeIfSignedImpl
-{
-    static T run(T x) { return -x; }
-};
+template<typename T, bool IsSigned = IsSigned<T>::value>
+struct NegateImpl;
+
 template<typename T>
-struct OppositeIfSignedImpl<T, false>
+struct NegateImpl<T, false>
 {
-    static T run(T x) { return x; }
+    static CheckedInt<T> negate(const CheckedInt<T>& val)
+    {
+      // Handle negation separately for signed/unsigned, for simpler code and to
+      // avoid an MSVC warning negating an unsigned value.
+      return CheckedInt<T>(0, val.isValid() && val.mValue == 0);
+    }
 };
+
 template<typename T>
-inline T
-OppositeIfSigned(T x)
+struct NegateImpl<T, true>
 {
-  return OppositeIfSignedImpl<T>::run(x);
-}
+    static CheckedInt<T> negate(const CheckedInt<T>& val)
+    {
+      // Watch out for the min-value, which (with twos-complement) can't be
+      // negated as -min-value is then (max-value + 1).
+      if (!val.isValid() || val.mValue == MinValue<T>::value)
+        return CheckedInt<T>(val.mValue, false);
+      return CheckedInt<T>(-val.mValue, true);
+    }
+};
 
 } // namespace detail
 
 
 /*
  * Step 4: Now define the CheckedInt class.
  */
 
@@ -555,16 +566,18 @@ class CheckedInt
 
     template<typename U>
     CheckedInt(U value, bool isValid) : mValue(value), mIsValid(isValid)
     {
       MOZ_STATIC_ASSERT(detail::IsSupported<T>::value,
                         "This type is not supported by CheckedInt");
     }
 
+    friend class detail::NegateImpl<T>;
+
   public:
     /**
      * Constructs a checked integer with given @a value. The checked integer is
      * initialized as valid or invalid depending on whether the @a value
      * is in range.
      *
      * This constructor is not explicit. Instead, the type of its argument is a
      * separate template parameter, ensuring that no conversion is performed
@@ -623,24 +636,17 @@ class CheckedInt
     template<typename U>
     friend CheckedInt<U> operator /(const CheckedInt<U>& lhs,
                                     const CheckedInt<U> &rhs);
     template<typename U>
     CheckedInt& operator /=(U rhs);
 
     CheckedInt operator -() const
     {
-      // Circumvent msvc warning about - applied to unsigned int.
-      // if we're unsigned, the only valid case anyway is 0
-      // in which case - is a no-op.
-      T result = detail::OppositeIfSigned(mValue);
-      /* Help the compiler perform RVO (return value optimization). */
-      return CheckedInt(result,
-                        mIsValid && detail::IsSubValid(T(0),
-                                                       mValue));
+      return detail::NegateImpl<T>::negate(*this);
     }
 
     /**
      * @returns true if the left and right hand sides are valid
      * and have the same value.
      *
      * Note that these semantics are the reason why we don't offer
      * a operator!=. Indeed, we'd want to have a!=b be equivalent to !(a==b)
--- a/mfbt/tests/TestCheckedInt.cpp
+++ b/mfbt/tests/TestCheckedInt.cpp
@@ -192,25 +192,30 @@ void test()
 
   /* Unary operator- checks */
 
   const CheckedInt<T> negOne = -one;
   const CheckedInt<T> negTwo = -two;
 
   if (isTSigned) {
     VERIFY_IS_VALID(-max);
+    VERIFY_IS_INVALID(-min);
+    VERIFY(-max - min == one);
     VERIFY_IS_VALID(-max - one);
     VERIFY_IS_VALID(negOne);
     VERIFY_IS_VALID(-max + negOne);
     VERIFY_IS_VALID(negOne + one);
     VERIFY(negOne + one == zero);
     VERIFY_IS_VALID(negTwo);
     VERIFY_IS_VALID(negOne + negOne);
     VERIFY(negOne + negOne == negTwo);
   } else {
+    VERIFY_IS_INVALID(-max);
+    VERIFY_IS_VALID(-min);
+    VERIFY(min == zero);
     VERIFY_IS_INVALID(negOne);
   }
 
   /* multiplication checks */
 
   VERIFY_IS_VALID(zero * zero);
   VERIFY(zero * zero == zero);
   VERIFY_IS_VALID(zero * one);