logging: Provide a specific MakeCheckOpValueString overload for functions

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

Committed: https://crrev.com/81f2120f583ad96cf496cc258f51a9c177437485
Cr-Commit-Position: refs/heads/master@{#434702}

[jbroman, 2016-11-21 15:48:56]

It seems odd to add an operator<< overload in base/logging.h, since that has
other implications. If we want to do this, I'd expect an overload of
MakeCheckOpValueString instead, like the ones below.

[Nico, 2016-11-21 15:51:10]

lgtm

Add https://bugs.chromium.org/p/chromium/issues/detail?id=550065#c12 to BUG
line; I think that fixes a bunch of the warnings there. (Check that this does
the right thing for MSVC -- I think it already does the right thing, but check
that it still does the right thing then :-) You can test this on linux by
writing a small test .cc file and use
third_party/llvm-build/Release+Asserts/bin/clang-cl /c test.cc -Xclang
-ast-dump)

[Raphael Kubo da Costa (rakuco), 2016-11-21 16:10:35]

On 2016/11/21 15:48:56, jbroman wrote:
> It seems odd to add an operator<< overload in base/logging.h, since that has
> other implications. If we want to do this, I'd expect an overload of
> MakeCheckOpValueString instead, like the ones below.

That was my first idea, but I was a bit weary of doing it because of
readability, the number of overloads and maintainability: there needs to be at
least one overload for (base::internal::SupportsOstreamOperator<const T&>::value
&& std::is_function<typename std::remove_pointer<T>::type>::value), one for
(base::internal::SupportsOstreamOperator<const T&>::value &&
!std::is_function<typename std::remove_pointer<T>::type>::value) and one for
(!base::internal::SupportsOstreamOperator<const T&>::value &&
std::is_enum<T>::value), and extending the checks or adding new ones might leave
us with a large number of similar functions.

With that said, I get your point that overloading operator<< in logging.h may
cause unforeseen issues.

nico/jbroman: Should I switch to a MakeCheckOpValueString overload instead?

[Raphael Kubo da Costa (rakuco), 2016-11-21 16:11:55]

Hmm, win_chromium_rel_ng's output isn't very helpful, but it does look like I'll
have to check this out on Windows.

[jbroman, 2016-11-21 16:37:33]

On 2016/11/21 at 16:10:35, raphael.kubo.da.costa wrote:
> That was my first idea, but I was a bit weary of doing it because of
readability, the number of overloads and maintainability: there needs to be at
least one overload for (base::internal::SupportsOstreamOperator<const T&>::value
&& std::is_function<typename std::remove_pointer<T>::type>::value), one for
(base::internal::SupportsOstreamOperator<const T&>::value &&
!std::is_function<typename std::remove_pointer<T>::type>::value) and one for
(!base::internal::SupportsOstreamOperator<const T&>::value &&
std::is_enum<T>::value), and extending the checks or adding new ones might leave
us with a large number of similar functions.
> 
> With that said, I get your point that overloading operator<< in logging.h may
cause unforeseen issues.
> 
> nico/jbroman: Should I switch to a MakeCheckOpValueString overload instead?

It does make it slightly more complicated, yeah, but I think you'd only need to
qualify the current default one (to "SupportsOstreamOperator && !is_function"),
as the enum one already won't match function pointers. If it got much worse one
could imagine a tag dispatch approach, but (shrug).

[Raphael Kubo da Costa (rakuco), 2016-11-21 17:31:28]

I've uploaded a new version of the patch overloading MakeCheckOpValueString as
discussed. I've also triggered a few relevant trybots, and will check back
tomorrow; if everything works, I'll update the CL description and provide the
information Nico asked for.

[Raphael Kubo da Costa (rakuco), 2016-11-22 12:07:56]

Description was changed from

==========
logging: Add an operator<< overload for functions and function pointers

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.

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

to

==========
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.

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

[Raphael Kubo da Costa (rakuco), 2016-11-22 12:09:32]

Description was changed from

==========
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.

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

to

==========
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
==========

[Raphael Kubo da Costa (rakuco), 2016-11-22 12:36:38]

Patch v3 passes on release and debug configurations; I've also checked GCC 6.2.1
on Linux and MSVC. The CL description has also been updated.

I produced some output with clang-cl on Linux with a reduced test case (I also
had to pass some -I's for the STL headers to be found) and it looks like the
right overload was selected for both functions and non-functions (let me know if
you'd like me to paste the output here).

Are we good to go?

[jbroman, 2016-11-22 14:40:52]

non-owner lgtm, but you should probably have a base/ owner (danakj or thakis
would do) double-check that they're happy with the latest patchset

[Nico, 2016-11-23 18:09:26]

code looks still fine to me, but test needs minor tweaking:

https://codereview.chromium.org/2515283002/diff/40001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2515283002/diff/40001/base/logging.h#newcode538
base/logging.h:538: // implicitly converted to bool (MSVC seems to be the
exception) and produce
This comment is a bit confusing as written. I'd reword it as:

// 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.)

https://codereview.chromium.org/2515283002/diff/40001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2515283002/diff/40001/base/logging_unittest.c...
base/logging_unittest.cc:226: void DcheckEmptyFunction2() {}
nit: newline after this

https://codereview.chromium.org/2515283002/diff/40001/base/logging_unittest.c...
base/logging_unittest.cc:274: EXPECT_EQ(DCHECK_IS_ON() ? 1 : 0,
log_sink_call_count);
This is super confusing – it looks like fp1 and fp3 are different, but this is
here because of line 267. Instead of doing this, set log_sinc_call_count = 0
below your "Test DCHECK" comment, and then have a EXPECT_EQ(0,
log_sink_call_count) here

[Raphael Kubo da Costa (rakuco), 2016-11-23 18:14:53]

Done!

[Nico, 2016-11-23 18:21:10]

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.c...
base/logging_unittest.cc:276: EXPECT_EQ(DCHECK_IS_ON() ? 0 : 0,
log_sink_call_count);
Err, replace `DCHECK_IS_ON() ? 0 : 0` with `0`? :-)

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.c...
base/logging_unittest.cc:280: static void MemberFunction1(){};
Actually, this isn't a member function, this is a regular function since it's
static. I think you want:
  struct MemberFunctions {
    void MemberFunction1() {}
    void MemberFunction2() {}
  };
  void (MemberFunctions::* mp1)() = &MemberFunctions::MemberFunction1;
  void (MemberFunctions::* mp2)() = &MemberFunctions::MemberFunction2;
  DCHECK_EQ(fp1, &MemberFunctions::MemberFunction1);
  DCHECK_EQ(fp2, &MemberFunctions::MemberFunction1);

Also, I'd move the first of these checks (and the whole struct decl) up so that
you can EXPECT_EQ(0, log_sink_call_count) after the first DCHECK

[Raphael Kubo da Costa (rakuco), 2016-11-23 18:58:09]

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.c...
base/logging_unittest.cc:276: EXPECT_EQ(DCHECK_IS_ON() ? 0 : 0,
log_sink_call_count);
On 2016/11/23 18:21:10, Nico wrote:
> Err, replace `DCHECK_IS_ON() ? 0 : 0` with `0`? :-)

Brain fart :(

https://codereview.chromium.org/2515283002/diff/60001/base/logging_unittest.c...
base/logging_unittest.cc:280: static void MemberFunction1(){};
On 2016/11/23 18:21:10, Nico wrote:
> Actually, this isn't a member function, this is a regular function since it's
> static. I think you want:
>   struct MemberFunctions {
>     void MemberFunction1() {}
>     void MemberFunction2() {}
>   };
>   void (MemberFunctions::* mp1)() = &MemberFunctions::MemberFunction1;
>   void (MemberFunctions::* mp2)() = &MemberFunctions::MemberFunction2;
>   DCHECK_EQ(fp1, &MemberFunctions::MemberFunction1);
>   DCHECK_EQ(fp2, &MemberFunctions::MemberFunction1);
> 
> Also, I'd move the first of these checks (and the whole struct decl) up so
that
> you can EXPECT_EQ(0, log_sink_call_count) after the first DCHECK

That works too; the reason I went for static functions is that I only started
working on it because wtf's ArrayBufferContents does

    DCHECK_EQ(s_adjustAmountOfExternalAllocatedMemoryFunction,
              defaultAdjustAmountOfExternalAllocatedMemoryFunction);

Having a static variable will require me to move the struct declaration out of
the function, so I guess I'll just go with actual member functions.

[Raphael Kubo da Costa (rakuco), 2016-11-23 19:04:38]

Patch updated again, please take another look.

[Nico, 2016-11-28 15:08:32]

lgtm

[Raphael Kubo da Costa (rakuco), 2016-11-28 15:11:14]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2016-11-28 15:11:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2515283002/#ps80001
(title: "Use actual member functions")

[commit-bot: I haz the power, 2016-11-28 15:11:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 18:36:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480345874393620,
"parent_rev": "5bd2051a21d4e68bbe05667f50af22d5f20a21e7", "commit_rev":
"c5758608e77883cbf53ea15db53a78bcd5c34ba1"}

[commit-bot: I haz the power, 2016-11-28 18:37:10]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-28 18:37:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-28 18:43:43]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/81f2120f583ad96cf496cc258f51a9c177437485
Cr-Commit-Position: refs/heads/master@{#434702}
==========

[commit-bot: I haz the power, 2016-11-28 18:43:43]

Patchset 5 (id:??) landed as
https://crrev.com/81f2120f583ad96cf496cc258f51a9c177437485
Cr-Commit-Position: refs/heads/master@{#434702}