logging: Provide a specific MakeCheckOpValueString overload for functions

So far, passing a function or function pointer to a check such as DCHECK_EQ
would cause the values to be implicitly converted to bool, resulting in a
confusing message like this

    Check failed: x == y (1 vs. 1)

instead of

    Check failed: x == y (0x779410 vs. 0x778fe0)

Add a specific overload for functions and function pointers that explicitly
cast them to const void* to have them call the right operator<< overload.

This fixes warnings on both new GCC versions (-Waddress complains that passing a function or function pointer to operator<< would always become '1') and clang on Windows (see https://bugs.chromium.org/p/chromium/issues/detail?id=550065#c12).

BUG=550065
R=danakj@chromium.org,thakis@chromium.org,jbroman@chromium.org

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

base/logging.h

@@ -527,12 +527,26 @@ class CheckOpResult {
 // it uses the definition for operator<<, with a few special cases below.
 template <typename T>
 inline typename std::enable_if<
-    base::internal::SupportsOstreamOperator<const T&>::value,
+    base::internal::SupportsOstreamOperator<const T&>::value &&
+        !std::is_function<typename std::remove_pointer<T>::type>::value,
     void>::type
 MakeCheckOpValueString(std::ostream* os, const T& v) {
   (*os) << v;
 }
 
+// Provide an overload for functions and function pointers. Function pointers
+// don't implicitly convert to void* but do implicitly convert to bool, so
+// without this function pointers are always printed as 1 or 0. (MSVC isn't
+// standards-conforming here and converts function pointers to regular
+// pointers, so this is a no-op for MSVC.)
+template <typename T>
+inline typename std::enable_if<
+    std::is_function<typename std::remove_pointer<T>::type>::value,
+    void>::type
+MakeCheckOpValueString(std::ostream* os, const T& v) {
+  (*os) << reinterpret_cast<const void*>(v);
+}
+
 // We need overloads for enums that don't support operator<<.
 // (i.e. scoped enums where no operator<< overload was declared).
 template <typename T>

base/logging_unittest.cc

@@ -217,6 +217,14 @@ TEST_F(LoggingTest, DcheckStreamsAreLazy) {
 #endif
 }
 
+void DcheckEmptyFunction1() {
+  // Provide a body so that Release builds do not cause the compiler to
+  // optimize DcheckEmptyFunction1 and DcheckEmptyFunction2 as a single
+  // function, which breaks the Dcheck tests below.
+  LOG(INFO) << "DcheckEmptyFunction1";
+}
+void DcheckEmptyFunction2() {}
+
 TEST_F(LoggingTest, Dcheck) {
 #if defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)
   // Release build.
@@ -258,6 +266,31 @@ TEST_F(LoggingTest, Dcheck) {
   EXPECT_EQ(0, log_sink_call_count);
   DCHECK_EQ(Animal::DOG, Animal::CAT);
   EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, log_sink_call_count);
+
+  // Test DCHECK on functions and function pointers.
+  log_sink_call_count = 0;
+  struct MemberFunctions {
+    void MemberFunction1() {
+      // See the comment in DcheckEmptyFunction1().
+      LOG(INFO) << "Do not merge with MemberFunction2.";
+    }
+    void MemberFunction2() {}
+  };
+  void (MemberFunctions::*mp1)() = &MemberFunctions::MemberFunction1;
+  void (MemberFunctions::*mp2)() = &MemberFunctions::MemberFunction2;
+  void (*fp1)() = DcheckEmptyFunction1;
+  void (*fp2)() = DcheckEmptyFunction2;
+  void (*fp3)() = DcheckEmptyFunction1;
+  DCHECK_EQ(fp1, fp3);
+  EXPECT_EQ(0, log_sink_call_count);
+  DCHECK_EQ(mp1, &MemberFunctions::MemberFunction1);
+  EXPECT_EQ(0, log_sink_call_count);
+  DCHECK_EQ(mp2, &MemberFunctions::MemberFunction2);
+  EXPECT_EQ(0, log_sink_call_count);
+  DCHECK_EQ(fp1, fp2);
+  EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0, log_sink_call_count);
+  DCHECK_EQ(mp2, &MemberFunctions::MemberFunction1);
+  EXPECT_EQ(DCHECK_IS_ON() ? 2 : 0, log_sink_call_count);
 }
 
 TEST_F(LoggingTest, DcheckReleaseBehavior) {