[base] Define CHECK comparison for signed vs. unsigned

src/base/logging.h

Index: src/base/logging.h
diff --git a/src/base/logging.h b/src/base/logging.h
index eae8a7fdb6aa68a5343a38936a3059128c451e2e..3a436eb9a1bed05eb8175f62396a203d0449ef38 100644
--- a/src/base/logging.h
+++ b/src/base/logging.h
@@ -115,21 +115,73 @@ DEFINE_MAKE_CHECK_OP_STRING(char const*)
 DEFINE_MAKE_CHECK_OP_STRING(void const*)
 #undef DEFINE_MAKE_CHECK_OP_STRING
 
+template <typename Lhs, typename Rhs>

[titzer, 2016/11/24 12:57:31]

This is a nasty little template knot that took me a while to unravel.
A couple comments could really make this clearer.

Suggest: adding a comment that std::enable_if<> declares a function only if the
predicate is true and std::make_unsigned<T> picks the integral unsigned type for
the given signed type.

[Clemens Hammacher, 2016/11/24 13:46:08]

I added several comments, and the MAKE_UNSIGNED macro to increase readability.

+struct is_signed_vs_unsigned {
+  enum : bool {
+    value = std::is_integral<Lhs>::value && std::is_integral<Rhs>::value &&
+            std::is_signed<Lhs>::value && std::is_unsigned<Rhs>::value
+  };
+};
+template <typename Lhs, typename Rhs>
+struct is_unsigned_vs_signed : public is_signed_vs_unsigned<Rhs, Lhs> {};
+template <typename Lhs, typename Rhs>
+struct is_signed_unsigned_mismatch {
+  enum : bool {
+    value = std::is_integral<Lhs>::value && std::is_integral<Rhs>::value &&
+            std::is_signed<Lhs>::value != std::is_signed<Rhs>::value
+  };
+};
+
+// Specialize compare operators for signed vs. unsigned comparisons.
+#define DEFINE_SIGNED_MISMATCH_COMP(CHECK, NAME, IMPL)                  \
+  template <typename Lhs, typename Rhs>                                 \
+  V8_INLINE typename std::enable_if<CHECK<Lhs, Rhs>::value, bool>::type \
+      Cmp##NAME##Impl(Lhs const& lhs, Rhs const& rhs) {                 \
+    return IMPL;                                                        \
+  }
+DEFINE_SIGNED_MISMATCH_COMP(
+    is_signed_vs_unsigned, EQ,
+    lhs >= 0 && static_cast<typename std::make_unsigned<Lhs>::type>(lhs) == rhs)
+DEFINE_SIGNED_MISMATCH_COMP(
+    is_signed_vs_unsigned, LT,
+    lhs < 0 || static_cast<typename std::make_unsigned<Lhs>::type>(lhs) < rhs)
+DEFINE_SIGNED_MISMATCH_COMP(
+    is_signed_vs_unsigned, LE,
+    lhs <= 0 || static_cast<typename std::make_unsigned<Lhs>::type>(lhs) <= rhs)
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, NE, !CmpEQImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, GT, !CmpLEImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_signed_vs_unsigned, GE, !CmpLTImpl(lhs, rhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, EQ, CmpEQImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, NE, CmpNEImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, LT, CmpGTImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, LE, CmpGEImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GT, CmpLTImpl(rhs, lhs))
+DEFINE_SIGNED_MISMATCH_COMP(is_unsigned_vs_signed, GE, CmpLEImpl(rhs, lhs))
+
 // Helper functions for CHECK_OP macro.
 // The (float, float) and (double, double) instantiations are explicitly
 // externialized to ensure proper 32/64-bit comparisons on x86.
 #define DEFINE_CHECK_OP_IMPL(NAME, op)                                         \
   template <typename Lhs, typename Rhs>                                        \
+  V8_INLINE                                                                    \
+      typename std::enable_if<!is_signed_unsigned_mismatch<Lhs, Rhs>::value,   \

[titzer, 2016/11/24 12:57:31]

Do you really need the auxilliary is_signed_unsigned_mismatch struct? Couldn't
you just use an && here.

[Clemens Hammacher, 2016/11/24 13:46:07]

Good suggestion, thanks. Done.

+                              bool>::type                                      \
+          Cmp##NAME##Impl(typename PassType<Lhs>::type lhs,                    \
+                          typename PassType<Rhs>::type rhs) {                  \
+    return lhs op rhs;                                                         \
+  }                                                                            \
+  template <typename Lhs, typename Rhs>                                        \
   V8_INLINE std::string* Check##NAME##Impl(typename PassType<Lhs>::type lhs,   \
                                            typename PassType<Rhs>::type rhs,   \
                                            char const* msg) {                  \
-    return V8_LIKELY(lhs op rhs) ? nullptr                                     \
-                                 : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg); \
+    bool cmp = Cmp##NAME##Impl<Lhs, Rhs>(lhs, rhs);                            \
+    return V8_LIKELY(cmp) ? nullptr                                            \
+                          : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg);        \
   }                                                                            \
   extern template V8_BASE_EXPORT std::string* Check##NAME##Impl<float, float>( \
-      const float lhs, const float rhs, char const* msg);                      \
+      float lhs, float rhs, char const* msg);                                  \
   extern template V8_BASE_EXPORT std::string*                                  \
-      Check##NAME##Impl<double, double>(const double lhs, const double rhs,    \
+      Check##NAME##Impl<double, double>(double lhs, double rhs,                \
                                         char const* msg);
 DEFINE_CHECK_OP_IMPL(EQ, ==)
 DEFINE_CHECK_OP_IMPL(NE, !=)