base: Support using (D)CHECK on scoped enums.

base: Support using (D)CHECK on scoped enums.

BUG=555314
Committed: https://crrev.com/6bcfec42d8e7ce09ab8c20a487ce285d2a680b00
Cr-Commit-Position: refs/heads/master@{#396052}

[jbroman, 2016-05-20 19:53:19]

Description was changed from

==========
base: Support using (D)CHECK on scoped enums.

BUG=
==========

to

==========
base: Support using (D)CHECK on scoped enums.
==========

[jbroman, 2016-05-20 19:56:35]

jbroman@chromium.org changed reviewers:
+ danakj@chromium.org

[jbroman, 2016-05-20 19:56:36]

WDYT?

Pending MSVC accepting it, of course. (/me crosses fingers)

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode529
base/logging.h:529: decltype(void(std::declval<std::ostream&>()
"void" here because the type has to match the one above, and shouldn't depend on
what the return type of operator<< is, as long as that expression has a valid
type. (In C++17, I'd use std::void_t here, but being a little ad-hoc here seemed
fine.)

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode552
base/logging.h:552: static_assert(sizeof(T) <= sizeof(int64_t),
64 bits ought to be enough for anybody. :)

[dcheng, 2016-05-20 19:59:30]

Of course, this would be a lot easier with std::underlying_type.

Also, https://codereview.chromium.org/1436403003/ =P

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode554
base/logging.h:554: (*os) << static_cast<int64_t>(v);
What if the enum's underlying type is uint64_t? =)

[jbroman, 2016-05-20 20:05:38]

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode554
base/logging.h:554: (*os) << static_cast<int64_t>(v);
On 2016/05/20 at 19:59:30, dcheng wrote:
> What if the enum's underlying type is uint64_t? =)

It'll do the obvious thing (values over 2^63 will become their corresponding
negative values). I think that's probably OK since this is only for DCHECK
messages, and that's an extreme edge case. I consider it an improvement
regardless, though we could of course implement underlying_type in base if we
really wanted to (Skia has, delegating to std::underlying_type when present and
using __underlying_type for libstdc++4.6, since our compiler supports the
underlying primitive anyways). It seemed more work than necessary, when we could
just wait for libstdc++ 4.6 to go away.

[jbroman, 2016-05-20 20:06:40]

jbroman@chromium.org changed reviewers:
+ dcheng@chromium.org

[danakj, 2016-05-20 21:08:23]

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode554
base/logging.h:554: (*os) << static_cast<int64_t>(v);
On 2016/05/20 20:05:38, jbroman wrote:
> On 2016/05/20 at 19:59:30, dcheng wrote:
> > What if the enum's underlying type is uint64_t? =)
> 
> It'll do the obvious thing (values over 2^63 will become their corresponding
> negative values). I think that's probably OK since this is only for DCHECK
> messages, and that's an extreme edge case. I consider it an improvement
> regardless, though we could of course implement underlying_type in base if we
> really wanted to (Skia has, delegating to std::underlying_type when present
and
> using __underlying_type for libstdc++4.6, since our compiler supports the
> underlying primitive anyways). It seemed more work than necessary, when we
could
> just wait for libstdc++ 4.6 to go away.

Oh, that might actually be nice, I didn't know __underlying_type exists. We have
some other missing libstdc++ stuff in
https://code.google.com/p/chromium/codesearch#chromium/src/base/template_util...

4.6 keeps dragging on, if we can do better it doesn't seem like much work to do
so.

[danakj, 2016-05-20 21:54:16]

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode526
base/logging.h:526: struct SupportsOstreamOperator : std::false_type {};
Can you move this out of logging.h. template_util.h perhaps?

It seems like we name type trait things in this_style to match the stl mostly,
maybe this should do the same as supports_ostream_operator. No strong feelings
either way though..

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode546
base/logging.h:546: inline typename
std::enable_if<!SupportsOstreamOperator<const T&>::value &&
Random horrible thought: what would go wrong if we just used this overload
always when is_enum is true?

[jbroman, 2016-05-25 19:07:15]

Description was changed from

==========
base: Support using (D)CHECK on scoped enums.
==========

to

==========
base: Support using (D)CHECK on scoped enums.

BUG=555314
==========

[jbroman, 2016-05-25 19:08:04]

https://codereview.chromium.org/2001783002/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode526
base/logging.h:526: struct SupportsOstreamOperator : std::false_type {};
On 2016/05/20 at 21:54:16, danakj wrote:
> Can you move this out of logging.h. template_util.h perhaps?
> 
> It seems like we name type trait things in this_style to match the stl mostly,
maybe this should do the same as supports_ostream_operator. No strong feelings
either way though..

Put it in base::internal since it's internal to base anyway, and the trait stuff
we have there matches Chromium naming.

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode546
base/logging.h:546: inline typename
std::enable_if<!SupportsOstreamOperator<const T&>::value &&
On 2016/05/20 at 21:54:16, danakj wrote:
> Random horrible thought: what would go wrong if we just used this overload
always when is_enum is true?

We'd have to change the above condition to !std::is_enum<T>::value to avoid
multiple matching overloads, and we'd degrade behaviour of cases where a better
definition of operator<< than this one is available (some enums do define one).

I'd prefer to only substitute our own operator<< equivalent if none is already
defined, personally.

https://codereview.chromium.org/2001783002/diff/1/base/logging.h#newcode554
base/logging.h:554: (*os) << static_cast<int64_t>(v);
On 2016/05/20 at 21:08:23, danakj wrote:
> On 2016/05/20 20:05:38, jbroman wrote:
> > On 2016/05/20 at 19:59:30, dcheng wrote:
> > > What if the enum's underlying type is uint64_t? =)
> > 
> > It'll do the obvious thing (values over 2^63 will become their corresponding
> > negative values). I think that's probably OK since this is only for DCHECK
> > messages, and that's an extreme edge case. I consider it an improvement
> > regardless, though we could of course implement underlying_type in base if
we
> > really wanted to (Skia has, delegating to std::underlying_type when present
and
> > using __underlying_type for libstdc++4.6, since our compiler supports the
> > underlying primitive anyways). It seemed more work than necessary, when we
could
> > just wait for libstdc++ 4.6 to go away.
> 
> Oh, that might actually be nice, I didn't know __underlying_type exists. We
have some other missing libstdc++ stuff in
https://code.google.com/p/chromium/codesearch#chromium/src/base/template_util...
> 
> 4.6 keeps dragging on, if we can do better it doesn't seem like much work to
do so.

Done, with the same semi-hacky condition Skia uses.

[danakj, 2016-05-25 19:53:00]

LGTM

[jbroman, 2016-05-25 19:57:09]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-05-25 19:57:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001783002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001783002/100001

[jbroman, 2016-05-25 20:10:45]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-05-25 20:11:45]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-05-25 20:11:45]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2001783002/#ps120001
(title: "use CR_GLIBCXX_4_7_0 macro, since I bothered to define it")

[commit-bot: I haz the power, 2016-05-25 20:12:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2001783002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2001783002/120001

[commit-bot: I haz the power, 2016-05-26 00:28:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-26 00:30:39]

Description was changed from

==========
base: Support using (D)CHECK on scoped enums.

BUG=555314
==========

to

==========
base: Support using (D)CHECK on scoped enums.

BUG=555314

Committed: https://crrev.com/6bcfec42d8e7ce09ab8c20a487ce285d2a680b00
Cr-Commit-Position: refs/heads/master@{#396052}
==========

[commit-bot: I haz the power, 2016-05-26 00:30:40]

Patchset 7 (id:??) landed as
https://crrev.com/6bcfec42d8e7ce09ab8c20a487ce285d2a680b00
Cr-Commit-Position: refs/heads/master@{#396052}

[Nico, 2016-05-26 00:44:54]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-26 00:44:55]

https://codereview.chromium.org/2001783002/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2001783002/diff/120001/base/logging.h#newcode566
base/logging.h:566: // in logging.cc.
Hm, this will never fire for enum classes since they're distinct types. (I guess
for enums it didn't fire either since we don't have a bare int specialization).

Still, this adds a bunch of includes and a bunch of code to a very heavily
included header. Have you checked if this impacts compile time?

[jbroman, 2016-05-26 15:30:43]

On 2016/05/26 at 00:44:55, thakis wrote:
> https://codereview.chromium.org/2001783002/diff/120001/base/logging.h
> File base/logging.h (right):
> 
>
https://codereview.chromium.org/2001783002/diff/120001/base/logging.h#newcode566
> base/logging.h:566: // in logging.cc.
> Hm, this will never fire for enum classes since they're distinct types. (I
guess for enums it didn't fire either since we don't have a bare int
specialization).

This CL shouldn't change anything about how those are instantiated, since it
only affects the functions that are called for cases where operator<< would
previously have failed to compile.

Besides, I believe even ordinary enums don't select the int specialization here,
because the conversion is only needed to invoke
std::basic_ostream::operator<<(int), later on.

> Still, this adds a bunch of includes and a bunch of code to a very heavily
included header. Have you checked if this impacts compile time?

I suspect nearly all files already include these headers (<iosfwd>,
<type_traits> and <utility>), because if you have base/logging.h and
base/memory/ref_counted.h (for instance), you get them already. An quick
(unscientific) test locally doesn't show any impact (clean but warm debug build
of base_unittests and its dependencies without goma on a Z620):

with this patch:
ninja -C out/A base_unittests  1207.64s user 70.56s system 2729% cpu 46.828
total

with this patch reverted:
ninja -C out/A base_unittests  1205.01s user 71.42s system 2713% cpu 47.044
total