This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]