Re: [PATCH 2/3] enum_flags: Fix problems and add comprehensive unit tests

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2016-11-03 23:22, Pedro Alves wrote:
> This patch starts by adding comprehensive unit tests for enum_flags.
> 
> It adds:
> 
>  - tests of normal expected uses of the API.
> 
>  - checks that _invalid_ uses of the API would fail to compile.  I.e.,
>    it validates that enum_flags really is a strong type, and that
>    incorrect mixing of enum types would be caught at compile time.  It
>    pulls that off making use of SFINEA and C++11's decltype/constexpr.
> 
> This revealed many holes in the enum_flags API.  For example, the f1
> assignment below currently incorrectly fails to compile:
> 
>  enum_flags<flags> f1 = FLAG1;
>  enum_flags<flags> f2 = FLAG2 | f1;
> 
> This hole and more are all plugged by this patch, by reworking how the
> enum_flags operators are implemented, and making use of C++11's
> feature of being able to delete methods/functions.
> 
> This makes most of the enum_flags operators constexpr.  Beyond
> enabling more compiler optimizations and enabling the new unit tests,
> this has other advantages, like making it possible to use operator|
> with enum_flags values in switch cases, where only compile-time
> constants are allowed:
> 
>     enum_flags<flags> f = FLAG1 | FLAG2;
>     switch (f)
>       {
>       case FLAG1 | FLAG2:
> 	break;
>       }
> 
> Currently that fails to compile.
> 
> This adds a STATIC_SELF_TEST macro to selftest.h, which is a variant
> of SELF_TEST, but uses C++11's static_assert to do checking at compile
> time.
> 
> To avoid potential ODR violations, since the tests add enums with
> short names that could easily conflict with other names, the new
> selftests are placed in a namespace (selftests::enum_flags_tests).  I
> think that's a good practice that we should start following.  This
> required splitting the global operator overload enablement out of the
> DEF_ENUM_FLAGS_TYPE macro into a separate macro, because
> DEF_ENUM_FLAGS_TYPE wants to create the enum flags typedef in the
> global namespace too.
> 
> Tested with gcc 4.8, 4.9, 5.3, 7 (trunk) and clang 3.7.

Yay, more unit tests!

I don't understand much about the internal implementation, but as long 
as it makes the class easy to use correctly and hard to use incorrectly, 
I think it's a win.

As a side-note, there's still the C implementation at the bottom of this 
file, it can probably go away.

Thanks,

Simon