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

Simon Marchi
2016-11-05T02:47:43Z

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