Make scoped enums output streamable.

Make scoped enums output streamable.

This lets scoped enums be used in various CHECK_* expressions,
as well as streamed to LOG(x).

BUG=555314

[dcheng, 2015-11-13 03:30:10]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org, thakis@chromium.org

[dcheng, 2015-11-13 03:30:24]

A blatant abuse of template metaprogramming.

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

https://codereview.chromium.org/1436403003/diff/1/base/logging.h#newcode12
base/logging.h:12: #include <type_traits>
Pretty sure this isn't allowed yet.

https://codereview.chromium.org/1436403003/diff/1/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/1436403003/diff/1/base/logging_unittest.cc#ne...
base/logging_unittest.cc:267: }
This is where no-compile tests would be really useful, but I have no idea when
the last time anyone ran the no-compile tests was.

[dcheng, 2015-11-13 19:38:46]

So it turns out this doesn't compile on many bots.

What version of libstdc++/libc++ are we using? I thought std::underlying_type
was a C++11 feature.

[Nico, 2015-11-13 19:57:12]

On 2015/11/13 19:38:46, dcheng wrote:
> So it turns out this doesn't compile on many bots.
> 
> What version of libstdc++/libc++ are we using? I thought std::underlying_type
> was a C++11 feature.

On linux, libstdc++4.6. libc++ headers are ~trunk on Mac, whatever's in the NDK
on Android (~recent) except on the android/clang bots where it's ~trunk. (For
clang, libc++ headers ship with the compiler). On CrOS, a pretty new libstdc++
(4.8? 5.0?)

On
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
the error is 

../../ui/touch_selection/touch_handle.cc:48:22:error: 'std::ostream&
ui::operator<<(std::ostream&, const ui::TouchHandleOrientation&)' defined but
not used [-Werror=unused-function]
 static std::ostream& operator<<(std::ostream& os,
                      ^

which looks like a different problem (but probably also due to this CL). This is
also what Mac complains about.

On Windows, it's

FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool action-wrapper
environment.x86
remoting_client_plugin_nacl_target_build_newlib_pexe_b216360c2163a114a01230d9e0df46a0..rsp
..\..\remoting
../out/Debug/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a:error:
undefined reference to 'std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*
logging::MakeCheckOpString<int, int>(int const&, int const&, char const*)'

FAILED with 1: ..\native_client\toolchain\win_x86\pnacl_newlib\bin\pnacl-clang++
-o ..\out\Debug/remoting_client_plugin_newlib.pexe.debug -Wl,--as-needed
..\out\Debug\obj\remoting\remoting_client_plugin_nacl.gen/pnacl_newlib/remoting_client_plugin_nacl\pepper_module_72e86210.o
-B..\out\Debug/gen/tc_pnacl_newlib/lib -O3 -lppapi_stub
-lremoting_client_plugin_lib_nacl -lremoting_proto_nacl -ljingle_glue_nacl
-lnet_nacl -lcrypto_nacl -lbase_i18n_nacl -lbase_nacl -lurl_nacl
-lremoting_webrtc_nacl -lyuv_nacl -lvpx_nacl -ljingle_p2p_constants_nacl
-ljingle_nacl -lexpat_nacl -lmodp_b64_nacl -lopus_nacl -lboringssl_nacl
-licui18n_nacl -licuuc_nacl -licudata_nacl -lprotobuf_lite_nacl -lppapi_cpp
-lpthread -lnacl_io


which I guess points to some pnacl build config bug?


On Android, it looks like it does complain about std::underlying_type. libc++'s
headers say:

#ifdef _LIBCPP_UNDERLYING_TYPE

template <class _Tp>
struct underlying_type
{
    typedef _LIBCPP_UNDERLYING_TYPE(_Tp) type;
};


and

#if __clang__
#if __has_feature(underlying_type)
#  define _LIBCPP_UNDERLYING_TYPE(T) __underlying_type(T)
#endif
#elif defined(__GNUC__)
#if _GNUC_VER >= 407
#define _LIBCPP_UNDERLYING_TYPE(T) __underlying_type(T)
#endif
#endif

So that should work I would've though – would be good to investigate locally
what exactly's going wrong here.

[danakj, 2015-11-13 19:57:33]

On Fri, Nov 13, 2015 at 11:38 AM, <dcheng@chromium.org> wrote:

> So it turns out this doesn't compile on many bots.
>
> What version of libstdc++/libc++ are we using? I thought
> std::underlying_type
> was a C++11 feature.
>

http://en.cppreference.com/w/cpp/types/underlying_type

If T is a complete (since C++17) enumeration type, provides a member
typedef type that names the underlying type of T.

^ maybe the C++17 part is relevant to you here?


>
> https://codereview.chromium.org/1436403003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-11-13 20:01:50]

On 2015/11/13 at 19:57:33, danakj wrote:
> On Fri, Nov 13, 2015 at 11:38 AM, <dcheng@chromium.org> wrote:
> 
> > So it turns out this doesn't compile on many bots.
> >
> > What version of libstdc++/libc++ are we using? I thought
> > std::underlying_type
> > was a C++11 feature.
> >
> 
> http://en.cppreference.com/w/cpp/types/underlying_type
> 
> If T is a complete (since C++17) enumeration type, provides a member
> typedef type that names the underlying type of T.

I believe it's stating that the C++17 standard added a clarification here (so
C++17 requires that the enumeration type be complete as well).

> 
> ^ maybe the C++17 part is relevant to you here?
> 
> 
> >
> > https://codereview.chromium.org/1436403003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
>

[dcheng, 2015-11-13 20:03:24]

On 2015/11/13 at 19:57:12, thakis wrote:
> On 2015/11/13 19:38:46, dcheng wrote:
> > So it turns out this doesn't compile on many bots.
> > 
> > What version of libstdc++/libc++ are we using? I thought
std::underlying_type
> > was a C++11 feature.
> 
> On linux, libstdc++4.6. libc++ headers are ~trunk on Mac, whatever's in the
NDK on Android (~recent) except on the android/clang bots where it's ~trunk.
(For clang, libc++ headers ship with the compiler). On CrOS, a pretty new
libstdc++ (4.8? 5.0?)
> 
> On
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...
the error is 
> 
> ../../ui/touch_selection/touch_handle.cc:48:22:error: 'std::ostream&
ui::operator<<(std::ostream&, const ui::TouchHandleOrientation&)' defined but
not used [-Werror=unused-function]
>  static std::ostream& operator<<(std::ostream& os,
>                       ^
> 
> which looks like a different problem (but probably also due to this CL). This
is also what Mac complains about.
> 
> On Windows, it's
> 
> FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool action-wrapper
environment.x86
remoting_client_plugin_nacl_target_build_newlib_pexe_b216360c2163a114a01230d9e0df46a0..rsp
..\..\remoting
>
../out/Debug/gen/tc_pnacl_newlib/lib/libremoting_client_plugin_lib_nacl.a:error:
undefined reference to 'std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> >*
logging::MakeCheckOpString<int, int>(int const&, int const&, char const*)'
> 
> FAILED with 1:
..\native_client\toolchain\win_x86\pnacl_newlib\bin\pnacl-clang++ -o
..\out\Debug/remoting_client_plugin_newlib.pexe.debug -Wl,--as-needed
..\out\Debug\obj\remoting\remoting_client_plugin_nacl.gen/pnacl_newlib/remoting_client_plugin_nacl\pepper_module_72e86210.o
-B..\out\Debug/gen/tc_pnacl_newlib/lib -O3 -lppapi_stub
-lremoting_client_plugin_lib_nacl -lremoting_proto_nacl -ljingle_glue_nacl
-lnet_nacl -lcrypto_nacl -lbase_i18n_nacl -lbase_nacl -lurl_nacl
-lremoting_webrtc_nacl -lyuv_nacl -lvpx_nacl -ljingle_p2p_constants_nacl
-ljingle_nacl -lexpat_nacl -lmodp_b64_nacl -lopus_nacl -lboringssl_nacl
-licui18n_nacl -licuuc_nacl -licudata_nacl -lprotobuf_lite_nacl -lppapi_cpp
-lpthread -lnacl_io
> 
> 
> which I guess points to some pnacl build config bug?
> 
> 
> On Android, it looks like it does complain about std::underlying_type.
libc++'s headers say:
> 
> #ifdef _LIBCPP_UNDERLYING_TYPE
> 
> template <class _Tp>
> struct underlying_type
> {
>     typedef _LIBCPP_UNDERLYING_TYPE(_Tp) type;
> };
> 
> 
> and
> 
> #if __clang__
> #if __has_feature(underlying_type)
> #  define _LIBCPP_UNDERLYING_TYPE(T) __underlying_type(T)
> #endif
> #elif defined(__GNUC__)
> #if _GNUC_VER >= 407
> #define _LIBCPP_UNDERLYING_TYPE(T) __underlying_type(T)
> #endif
> #endif
> 
> So that should work I would've though – would be good to investigate locally
what exactly's going wrong here.

The highly authoritative Stack overflow answers I found indicate that you need
libstdc++ 4.7 for std::underlying_type to be defined. So I guess that might be a
problem.

The other issues (I noticed the Windows one, missed the Mac one though) are
probably still worth investigating. I can still write this without
underlying_type, just not nearly as elegantly.

[Nico, 2015-11-13 20:09:43]

android needs investigating too; I think that's the most interesting failure.

Good to know about 4.6 not having this trait – probably good to have a list of
stuff that 4.6 is missing somewhere… (if it's a ton of stuff, I suppose we could
statically link libc++ there too?)

[dcheng, 2015-11-16 18:35:04]

On 2015/11/13 at 20:09:43, thakis wrote:
> android needs investigating too; I think that's the most interesting failure.
> 
> Good to know about 4.6 not having this trait – probably good to have a list of
stuff that 4.6 is missing somewhere… (if it's a ton of stuff, I suppose we could
statically link libc++ there too?)

I haven't really figured out the linking error. Maybe it's because I'm mixing
default template parameters with specialization and some of the tooling gets
confused?
I also haven't figured out the libc++ problem with std::underlying_type not
being reported as defined. I plan on experimenting with this more...

In the mean time, here's an alternate proposal which works around the
aforementioned issues:
- Instead of using std::underlying_type, it just uses std::is_signed and casts
to the maximum width integer of that signed-ness.
- Instead of modifying MakeCheckOpString(), it just enables all scoped enums to
be streamable to std::ostream.

The advantage is that you can do LOG(ERROR) << my_scoped_enum_value.
The disadvantage is that this will conflict with any user-defined std::ostream
for a particular scoped_enum (e.g. the one in touch_handle.cc).

Does this feel too magical? To me, it feels like something that would be
generally useful, especially since I like to encourage people to use scoped
enums.

Also, it turns out it's also possible to use SFINAE to disable operator<< for
any scoped enum that's already streamable (see internal CL 65845412 for a
proposal to do this in google3). So we could do that here as well, if we felt
this was an important use case to support (it doesn't seem to be though, as
there's only one instance of this pattern in Chromium code).

[danakj, 2015-11-20 19:42:28]

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

https://codereview.chromium.org/1436403003/diff/60001/base/logging_unittest.c...
base/logging_unittest.cc:263: enum class TestEnum {
can you check fixed signed and unsigned int also? what happens with fixed
size_t?

[danakj, 2015-11-20 19:43:49]

This fails for me:

enum class E : size_t { kFoo };

TEST(Dana, Dana) {
  bool b = std::is_convertible<E, int>::value;
  EXPECT_TRUE(b);
}

But enum without the "class" works.

[dcheng, 2015-11-20 20:03:41]

On 2015/11/20 at 19:43:49, danakj wrote:
> This fails for me:
> 
> enum class E : size_t { kFoo };
> 
> TEST(Dana, Dana) {
>   bool b = std::is_convertible<E, int>::value;
>   EXPECT_TRUE(b);
> }
> 
> But enum without the "class" works.

Yeah, std::is_convertible<E, int>::value is always true if E is an unscoped enum
and always false if E is a soped enum.

So I (abuse) this fact in conjunction with is_enum to define operator<< only for
scoped enums. That lets it work as expected with LOG(ERROR), CHECK_EQ(e1, e2),
etc.

[danakj, 2015-11-20 21:20:03]

On Fri, Nov 20, 2015 at 12:03 PM, <dcheng@chromium.org> wrote:

> On 2015/11/20 at 19:43:49, danakj wrote:
>
>> This fails for me:
>>
>
> enum class E : size_t { kFoo };
>>
>
> TEST(Dana, Dana) {
>>    bool b = std::is_convertible<E, int>::value;
>>    EXPECT_TRUE(b);
>> }
>>
>
> But enum without the "class" works.
>>
>
> Yeah, std::is_convertible<E, int>::value is always true if E is an
> unscoped enum
> and always false if E is a soped enum.
>
> So I (abuse) this fact in conjunction with is_enum to define operator<<
> only for
> scoped enums. That lets it work as expected with LOG(ERROR), CHECK_EQ(e1,
> e2),
> etc.
>

I see, and when is_convertible<E, int> is true then we don't need to do
this anyways.

I was mostly worried that "int" would not work sometimes but some other
basic type would.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-11-20 21:33:58]

ok I think I understand all the things and this looks good and helpful. few last
comments

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h#newcode518
base/logging.h:518: // Type trait helper to detect scoped enums.
Can you add comments here about how unscoped enums will already convert to ints
(or their fixed type), so they won't match this rule and will "just work"?

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h#newcode530
base/logging.h:530: os << static_cast<intmax_t>(v);
can you mention underlying_type with a TODO here and in the unsigned one?

[dcheng, 2015-11-23 18:42:13]

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h#newcode518
base/logging.h:518: // Type trait helper to detect scoped enums.
On 2015/11/20 at 21:33:58, danakj (behind on reviews) wrote:
> Can you add comments here about how unscoped enums will already convert to
ints (or their fixed type), so they won't match this rule and will "just work"?

Done.

https://codereview.chromium.org/1436403003/diff/60001/base/logging.h#newcode530
base/logging.h:530: os << static_cast<intmax_t>(v);
On 2015/11/20 at 21:33:58, danakj (behind on reviews) wrote:
> can you mention underlying_type with a TODO here and in the unsigned one?

Done (though I just made it a top-level comment about these two functions, to
avoid being flagged by the Department of Redundancy Department).

[dcheng, 2015-11-23 18:43:09]

Description was changed from

==========
Enable CHECK_EQ, CHECK_NE, etc to be used with scoped enums.

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

to

==========
Make scoped enums output streamable.

This lets scoped enums be used in various CHECK_* expressions,
as well as streamed to LOG(x).

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

[danakj, 2015-11-23 18:52:19]

thanks lgtm. just make the test cover both signed/unsigned please.

[dcheng, 2015-11-23 23:03:17]

I can fix the issue with std::is_signed always returning false for enums.

However, there's a more fundamental issue: I noticed that in this same file, we
already have operator<< for wstrings... in namespace std. After asking around,
mdempsky@ and rsleevi@ kindly reminded me this is important for ADL.

So we have two choices...
- Put scoped enum's operator<< directly into namespace std. This is undefined
behavior, according to the standard, but it will also permit scoped_enums to be
streamed to LOG(x).
- Take a more GTest-like approach and use helper functions to output to the
stream, providing specializations for any interesting cases. I think that means
we can really only make this work for CHECK_EQ, etc, but maybe that's really the
most important case?

[danakj, 2016-01-07 00:21:38]

On Mon, Nov 23, 2015 at 3:03 PM, <dcheng@chromium.org> wrote:

> I can fix the issue with std::is_signed always returning false for enums.
>
> However, there's a more fundamental issue: I noticed that in this same
> file, we
> already have operator<< for wstrings... in namespace std. After asking
> around,
> mdempsky@ and rsleevi@ kindly reminded me this is important for ADL.
>
> So we have two choices...
> - Put scoped enum's operator<< directly into namespace std. This is
> undefined
> behavior, according to the standard, but it will also permit scoped_enums
> to be
> streamed to LOG(x).
> - Take a more GTest-like approach and use helper functions to output to the
> stream, providing specializations for any interesting cases. I think that
> means
> we can really only make this work for CHECK_EQ, etc, but maybe that's
> really the
> most important case?
>

Yes I think that is okay. LOG() isn't so common and people can cast if they
have to. Making _EQ work would be pretty helpful tho, cuz that leaves silly
casts all over our test code otherwise.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2016-04-15 08:41:46]

So I spent some more time tinkering with this. First, the good news:
Signed vs unsigned detection actually works now. So the streaming bits all work.

Now for the bad news:
Adding a default MakeCheckOpValueString template means I need to use SFINAE to
disable it for special cases, i.e. scoped enums, now: otherwise, it won't be
able to select the right overload. But it's pretty ugly to have to do this.

Also, it looks like a lot of the custom enum class operator<<'s output the
result as the name of the enumerator. Forcing them to dump as an enum seems like
a net loss. Maybe this is balanced out by not having to static_cast…?

To mitigate the previous two bits of bad news, we could double down on the
template metaprogramming and add a type trait helper to determine if a given
type is streamable. Using this new type trait, we can then:
- Guard the default MakeCheckOpValueString with is_output_streamable<T>.
- Guard the scoped enum MakeCheckOpValueString with is_scoped_enum<T> &&
!is_output_streamable<T>.

Note: there are also some further improvements I could make in terms of
readability, but I'm holding off until we've decided that we actually want this
complexity…

(Also, another interesting thing I noted is that google3 forces
MakeCheckOpString to be out-of-lined: I wonder if we want to do that? Probably?)

[dcheng, 2016-04-15 21:14:10]

The more I work on this, the less useful I feel it is. It's been a couple
months, and there hasn't been overwhelming demand. There's a lot of template
magic involved, and it makes things generally harder to understand.

Given that, I'm just going to close this: if someone wants to champion this and
pick it up, feel free to: the complexity-payoff ratio just seems too high.