Revert of [base] Pass scalar arguments by value in CHECK/DCHECK (patchset #3...

Revert of [base] Pass scalar arguments by value in CHECK/DCHECK (patchset #3 id:40001 of https://codereview.chromium.org/2524093002/ )

Reason for revert:
Seems to cause compile errors on Android. Will investigate on Monday.

Original issue's description:
> [base] Pass scalar arguments by value in CHECK/DCHECK
>
> This not only potentially improves performance, but also avoids weird
> linker errors, like the one below, where I used Smi::kMinValue in a
> DCHECK_EQ.
>
> > [421/649] LINK ./mksnapshot
> > FAILED: mksnapshot
> > src/base/logging.h|178| error: undefined reference to
>   'v8::internal::Smi::kMinValue'
>
> R=bmeurer@chromium.org, ishell@chromium.org
>
> Committed: https://crrev.com/76723502528c5af003fdffc3520632ea2a13fef3
> Cr-Commit-Position: refs/heads/master@{#41273}

TBR=bmeurer@chromium.org,ishell@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

src/base/logging.cc

@@ -15,7 +15,8 @@ namespace base {
 
 // Explicit instantiations for commonly used comparisons.
 #define DEFINE_MAKE_CHECK_OP_STRING(type)              \
-  template std::string* MakeCheckOpString<type, type>(type, type, char const*);
+  template std::string* MakeCheckOpString<type, type>( \
+      type const&, type const&, char const*);
 DEFINE_MAKE_CHECK_OP_STRING(int)
 DEFINE_MAKE_CHECK_OP_STRING(long)       // NOLINT(runtime/int)
 DEFINE_MAKE_CHECK_OP_STRING(long long)  // NOLINT(runtime/int)
@@ -29,10 +30,10 @@ DEFINE_MAKE_CHECK_OP_STRING(void const*)
 
 // Explicit instantiations for floating point checks.
 #define DEFINE_CHECK_OP_IMPL(NAME)                          \
-  template std::string* Check##NAME##Impl<float, float>(float lhs, float rhs, \
-                                                        char const* msg);     \
+  template std::string* Check##NAME##Impl<float, float>(    \
+      float const& lhs, float const& rhs, char const* msg); \
   template std::string* Check##NAME##Impl<double, double>(  \
-      double lhs, double rhs, char const* msg);
+      double const& lhs, double const& rhs, char const* msg);
 DEFINE_CHECK_OP_IMPL(EQ)
 DEFINE_CHECK_OP_IMPL(NE)
 DEFINE_CHECK_OP_IMPL(LE)

src/base/logging.h

@@ -57,8 +57,7 @@ namespace base {
 // Don't use this macro directly in your code, use CHECK_EQ et al below.
 #define CHECK_OP(name, op, lhs, rhs)                                    \
   do {                                                                  \
-    if (std::string* _msg =                                              \
-            ::v8::base::Check##name##Impl<decltype(lhs), decltype(rhs)>( \
+    if (std::string* _msg = ::v8::base::Check##name##Impl(              \
             (lhs), (rhs), #lhs " " #op " " #rhs)) {                     \
       V8_Fatal(__FILE__, __LINE__, "Check failed: %s.", _msg->c_str()); \
       delete _msg;                                                      \
@@ -74,26 +73,13 @@ namespace base {
 
 #endif
 
-// Helpers to determine how to pass values: Pass scalars by value, others by
-// const reference.
-template <typename Lhs, typename Rhs>
-struct PassByValue {
-  enum : bool {
-    value = (std::is_scalar<Lhs>::value || std::is_array<Lhs>::value) &&
-            (std::is_scalar<Rhs>::value || std::is_array<Rhs>::value)
-  };
-};
-template <typename T>
-struct PassType
-    : public std::conditional<PassByValue<T, T>::value, T, T const&> {};
 
 // Build the error message string.  This is separate from the "Impl"
 // function template because it is not performance critical and so can
 // be out of line, while the "Impl" code should be inline. Caller
 // takes ownership of the returned string.
 template <typename Lhs, typename Rhs>
-std::string* MakeCheckOpString(typename PassType<Lhs>::type lhs,
-                               typename PassType<Rhs>::type rhs,
+std::string* MakeCheckOpString(Lhs const& lhs, Rhs const& rhs,
                                char const* msg) {
   std::ostringstream ss;
   ss << msg << " (" << lhs << " vs. " << rhs << ")";
@@ -104,7 +90,7 @@ std::string* MakeCheckOpString(typename PassType<Lhs>::type lhs,
 // in logging.cc.
 #define DEFINE_MAKE_CHECK_OP_STRING(type)                                    \
   extern template V8_BASE_EXPORT std::string* MakeCheckOpString<type, type>( \
-      type, type, char const*);
+      type const&, type const&, char const*);
 DEFINE_MAKE_CHECK_OP_STRING(int)
 DEFINE_MAKE_CHECK_OP_STRING(long)       // NOLINT(runtime/int)
 DEFINE_MAKE_CHECK_OP_STRING(long long)  // NOLINT(runtime/int)
@@ -115,21 +101,27 @@ DEFINE_MAKE_CHECK_OP_STRING(char const*)
 DEFINE_MAKE_CHECK_OP_STRING(void const*)
 #undef DEFINE_MAKE_CHECK_OP_STRING
 
+
 // Helper functions for CHECK_OP macro.
+// The (int, int) specialization works around the issue that the compiler
+// will not instantiate the template version of the function on values of
+// unnamed enum type - see comment below.
 // 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 std::string* Check##NAME##Impl(typename PassType<Lhs>::type lhs,   \
-                                           typename PassType<Rhs>::type rhs,   \
+  V8_INLINE std::string* Check##NAME##Impl(Lhs const& lhs, Rhs const& rhs,     \
+                                           char const* msg) {                  \
+    return V8_LIKELY(lhs op rhs) ? nullptr : MakeCheckOpString(lhs, rhs, msg); \
+  }                                                                            \
+  V8_INLINE std::string* Check##NAME##Impl(int lhs, int rhs,                   \
                                            char const* msg) {                  \
-    return V8_LIKELY(lhs op rhs) ? nullptr                                     \
-                                 : MakeCheckOpString<Lhs, Rhs>(lhs, rhs, msg); \
+    return V8_LIKELY(lhs op rhs) ? nullptr : MakeCheckOpString(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 const& lhs, float const& 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 const& lhs, double const& rhs,  \
                                         char const* msg);
 DEFINE_CHECK_OP_IMPL(EQ, ==)
 DEFINE_CHECK_OP_IMPL(NE, !=)

test/unittests/base/logging-unittest.cc

@@ -8,18 +8,11 @@
 namespace v8 {
 namespace base {
 
-namespace {
-template <typename Lhs, typename Rhs>
-std::string *CallCheckEQ(Lhs lhs, Rhs rhs, const char *msg) {
-  return CheckEQImpl<Lhs, Rhs>(lhs, rhs, msg);
-}
-}  // namespace
-
 TEST(LoggingTest, CheckEQImpl) {
-  EXPECT_EQ(nullptr, CallCheckEQ(0.0, 0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(0.0, -0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(-0.0, 0.0, ""));
-  EXPECT_EQ(nullptr, CallCheckEQ(-0.0, -0.0, ""));
+  EXPECT_EQ(nullptr, CheckEQImpl(0.0, 0.0, ""));
+  EXPECT_EQ(nullptr, CheckEQImpl(0.0, -0.0, ""));
+  EXPECT_EQ(nullptr, CheckEQImpl(-0.0, 0.0, ""));
+  EXPECT_EQ(nullptr, CheckEQImpl(-0.0, -0.0, ""));
 }
 
 }  // namespace base