This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Type-safe wrapper for enum flags
- From: Patrick Palka <patrick at parcs dot ath dot cx>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Sun, 1 Nov 2015 10:19:19 -0500
- Subject: Re: [PATCH] Type-safe wrapper for enum flags
- Authentication-results: sourceware.org; auth=none
- References: <1446144341-21267-1-git-send-email-palves at redhat dot com>
On Thu, Oct 29, 2015 at 2:45 PM, Pedro Alves <palves@redhat.com> wrote:
> This patch fixes C++ build errors like this:
>
> /home/pedro/gdb/mygit/cxx-convertion/src/gdb/linux-tdep.c:1126:35: error: invalid conversion from âintâ to âfilterflagsâ [-fpermissive]
> | COREFILTER_HUGETLB_PRIVATE);
> ^
>
> This is a case of enums used as bit flags. Unlike "regular" enums,
> these values are supposed to be or'ed together. However, in C++, the
> type of "(ENUM1 | ENUM2)" is int, and you then can't assign an int to
> an enum variable without a cast. That means that this:
>
> enum foo_flags flags = 0;
>
> if (...)
> flags |= FOO_FLAG1;
> if (...)
> flags |= FOO_FLAG2;
>
> ... would have to be written as:
>
> enum foo_flags flags = (enum foo_flags) 0;
>
> if (...)
> flags = (enum foo_flags) (flags | FOO_FLAG1);
> if (...)
> flags = (enum foo_flags) (flags | FOO_FLAG2);
>
> which is ... ugly. Alternatively, we'd have to use an int for the
> variable's type, which isn't ideal either.
>
> This patch instead adds an "enum flags" class. "enum flags" are
> exactly the enums where the values are bits that are meant to be ORed
> together.
>
> This allows writing code like the below, while with raw enums this
> would fail to compile without casts to enum type at the assignments to
> 'f':
>
> enum some_flag
> {
> flag_val1 = 1 << 1,
> flag_val2 = 1 << 2,
> flag_val3 = 1 << 3,
> flag_val4 = 1 << 4,
> };
> DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
>
> some_flags f = flag_val1 | flag_val2;
> f |= flag_val3;
>
> It's also possible to assign literal zero to an enum flags variable
> (meaning, no flags), dispensing either adding an awkward explicit "no
> value" value to the enumeration or the cast to assignments from 0.
> For example:
>
> some_flags f = 0;
> f |= flag_val3 | flag_val4;
>
> Note that literal integers other than zero do fail to compile:
>
> some_flags f = 1; // error
>
> C is still supported -- DEF_ENUM_FLAGS_TYPE is just a typedef in that
> case.
>
> gdb/ChangeLog:
> 2015-10-29 Pedro Alves <palves@redhat.com>
>
> * btrace.h: Include common/enum_flags.h.
> (btrace_insn_flags): Define.
> (struct btrace_insn) <flags>: Change type.
> (btrace_function_flags): Define.
> (struct btrace_function) <flags>: Change type.
> (btrace_thread_flags): Define.
> (struct btrace_thread_info) <flags>: Change type.
> * c-exp.y (token_flags): Rename to ...
> (token_flag): ... this.
> (token_flags): Define.
> (struct token) <flags>: Change type.
> * common/enum_flags.h: New file.
> * compile/compile-c-types.c (convert_qualified): Change type of
> 'quals' local.
> * compile/compile-internal.h: Include "common/enum_flags.h".
> (gcc_qualifiers_flags): Define.
> * completer.c (enum reg_completer_targets): Rename to ...
> (enum reg_completer_target): ... this.
> (reg_completer_targets): Define.
> (reg_or_group_completer_1): Change type of 'targets' parameter.
> * disasm.c (do_mixed_source_and_assembly_deprecated): Change type
> of 'psl_flags' local.
> (do_mixed_source_and_assembly): Change type of 'psl_flags' local.
> * infrun.c: Include "common/enum_flags.h".
> (enum step_over_what): Rename to ...
> (enum step_over_what_flag): ... this.
> (step_over_what): Change type.
> (start_step_over): Change type of 'step_what' local.
> (thread_still_needs_step_over): Now returns a step_over_what.
> Adjust.
> (keep_going_pass_signal): Change type of 'step_what' local.
> * linux-tdep.c: Include "common/enum_flags.h".
> (enum filterflags): Rename to ...
> (enum filter_flag): ... this.
> (filter_flags): Define.
> (dump_mapping_p): Change type of 'filterflags' parameter.
> (linux_find_memory_regions_full): Change type of 'filterflags'
> local.
> (linux_find_memory_regions_full): Pass the address of an unsigned
> int to sscanf instead of the address of an enum.
> * record-btrace.c (btrace_call_history): Replace 'flags' parameter
> with 'int_flags' parameter. Adjust.
> (record_btrace_call_history, record_btrace_call_history_range)
> (record_btrace_call_history_from): Rename 'flags' parameter to
> 'int_flags'. Use record_print_flags.
> * record.h: Include "common/enum_flags.h".
> (record_print_flags): Define.
> * source.c: Include "common/enum_flags.h".
> (print_source_lines_base, print_source_lines): Change type of
> flags parameter.
> * symtab.h: Include "common/enum_flags.h".
> (enum print_source_lines_flags): Rename to ...
> (enum print_source_lines_flag): ... this.
> (print_source_lines_flags): Define.
> (print_source_lines): Change prototype.
> ---
> gdb/btrace.h | 10 +-
> gdb/c-exp.y | 5 +-
> gdb/common/enum_flags.h | 211 +++++++++++++++++++++++++++++++++++++++++
> gdb/compile/compile-c-types.c | 2 +-
> gdb/compile/compile-internal.h | 4 +
> gdb/completer.c | 5 +-
> gdb/disasm.c | 4 +-
> gdb/infrun.c | 14 +--
> gdb/linux-tdep.c | 19 ++--
> gdb/record-btrace.c | 22 +++--
> gdb/record.h | 2 +
> gdb/source.c | 7 +-
> gdb/symtab.h | 6 +-
> 13 files changed, 275 insertions(+), 36 deletions(-)
> create mode 100644 gdb/common/enum_flags.h
>
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index f844df8..e012c1e 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -28,6 +28,7 @@
>
> #include "btrace-common.h"
> #include "target/waitstatus.h" /* For enum target_stop_reason. */
> +#include "common/enum_flags.h"
>
> #if defined (HAVE_LIBIPT)
> # include <intel-pt.h>
> @@ -58,6 +59,7 @@ enum btrace_insn_flag
> /* The instruction has been executed speculatively. */
> BTRACE_INSN_FLAG_SPECULATIVE = (1 << 0)
> };
> +DEF_ENUM_FLAGS_TYPE (enum btrace_insn_flag, btrace_insn_flags);
>
> /* A branch trace instruction.
>
> @@ -74,7 +76,7 @@ struct btrace_insn
> enum btrace_insn_class iclass;
>
> /* A bit vector of BTRACE_INSN_FLAGS. */
> - enum btrace_insn_flag flags;
> + btrace_insn_flags flags;
> };
>
> /* A vector of branch trace instructions. */
> @@ -100,6 +102,7 @@ enum btrace_function_flag
> if bfun_up_links_to_ret is clear. */
> BFUN_UP_LINKS_TO_TAILCALL = (1 << 1)
> };
> +DEF_ENUM_FLAGS_TYPE (enum btrace_function_flag, btrace_function_flags);
>
> /* Decode errors for the BTS recording format. */
> enum btrace_bts_error
> @@ -181,7 +184,7 @@ struct btrace_function
> int level;
>
> /* A bit-vector of btrace_function_flag. */
> - enum btrace_function_flag flags;
> + btrace_function_flags flags;
> };
>
> /* A branch trace instruction iterator. */
> @@ -245,6 +248,7 @@ enum btrace_thread_flag
> /* The thread is to be stopped. */
> BTHR_STOP = (1 << 4)
> };
> +DEF_ENUM_FLAGS_TYPE (enum btrace_thread_flag, btrace_thread_flags);
>
> #if defined (HAVE_LIBIPT)
> /* A packet. */
> @@ -342,7 +346,7 @@ struct btrace_thread_info
> unsigned int ngaps;
>
> /* A bit-vector of btrace_thread_flag. */
> - enum btrace_thread_flag flags;
> + btrace_thread_flags flags;
>
> /* The instruction history iterator. */
> struct btrace_insn_history *insn_history;
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index d093491..26050cb 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -2257,7 +2257,7 @@ parse_string_or_char (const char *tokptr, const char **outptr,
>
> /* This is used to associate some attributes with a token. */
>
> -enum token_flags
> +enum token_flag
> {
> /* If this bit is set, the token is C++-only. */
>
> @@ -2269,13 +2269,14 @@ enum token_flags
>
> FLAG_SHADOW = 2
> };
> +DEF_ENUM_FLAGS_TYPE (enum token_flag, token_flags);
>
> struct token
> {
> char *oper;
> int token;
> enum exp_opcode opcode;
> - enum token_flags flags;
> + token_flags flags;
> };
>
> static const struct token tokentab3[] =
> diff --git a/gdb/common/enum_flags.h b/gdb/common/enum_flags.h
> new file mode 100644
> index 0000000..030fd2f
> --- /dev/null
> +++ b/gdb/common/enum_flags.h
> @@ -0,0 +1,211 @@
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef COMMON_ENUM_FLAGS_H
> +#define COMMON_ENUM_FLAGS_H
> +
> +/* Type-safe wrapper for enum flags. enum flags are enums where the
> + values are bits that are meant to be ORed together.
> +
> + This allows writing code like the below, while with raw enums this
> + would fail to compile without casts to enum type at the assignments
> + to 'f':
> +
> + enum some_flag
> + {
> + flag_val1 = 1 << 1,
> + flag_val2 = 1 << 2,
> + flag_val3 = 1 << 3,
> + flag_val4 = 1 << 4,
> + };
> + DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags)
> +
> + some_flags f = flag_val1 | flag_val2;
> + f |= flag_val3;
> +
> + It's also possible to assign literal zero to an enum flags variable
> + (meaning, no flags), dispensing adding an awkward explicit "no
> + value" value to the enumeration. For example:
> +
> + some_flags f = 0;
> + f |= flag_val3 | flag_val4;
> +
> + Note that literal integers other than zero fail to compile:
> +
> + some_flags f = 1; // error
> +*/
> +
> +#ifdef __cplusplus
> +
> +/* Traits type used to prevent the global operator overloads from
> + instantiating for non-flag enums. */
> +template<typename T> struct enum_flags_type {};
> +
> +/* Use this to mark an enum as flags enum. It defines FLAGS as
> + enum_flags wrapper class for ENUM, and enables the global operator
> + overloads for ENUM. */
> +#define DEF_ENUM_FLAGS_TYPE(enum_type, flags_type) \
> + typedef enum_flags<enum_type> flags_type; \
> + template<> \
> + struct enum_flags_type<enum_type> \
> + { \
> + typedef enum_flags<enum_type> type; \
> + }
> +
> +/* Until we can rely on std::underlying type being universally
> + available (C++11), roll our own for enums. */
> +template<int size, bool sign> class integer_for_size { typedef void type; };
> +template<> struct integer_for_size<1, 0> { typedef uint8_t type; };
> +template<> struct integer_for_size<2, 0> { typedef uint16_t type; };
> +template<> struct integer_for_size<4, 0> { typedef uint32_t type; };
> +template<> struct integer_for_size<8, 0> { typedef uint64_t type; };
> +template<> struct integer_for_size<1, 1> { typedef int8_t type; };
> +template<> struct integer_for_size<2, 1> { typedef int16_t type; };
> +template<> struct integer_for_size<4, 1> { typedef int32_t type; };
> +template<> struct integer_for_size<8, 1> { typedef int64_t type; };
> +
> +template<typename T>
> +struct enum_underlying_type
> +{
> + typedef typename
> + integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
> + type;
> +};
> +
> +template <typename E>
> +class enum_flags
> +{
> +public:
> + typedef E enum_type;
> + typedef typename enum_underlying_type<enum_type>::type underlying_type;
> +
> +private:
> + /* Private type used to support initializing flag types with zero:
> +
> + foo_flags f = 0;
> +
> + but not other integers:
> +
> + foo_flags f = 1;
> +
> + The way this works is that we define an implicit constructor that
> + takes a pointer to this private type. Since nothing can
> + instantiate an object of this type, the only possible pointer to
> + pass to the constructor is the NULL pointer, or, zero. */
> + struct zero_type;
> +
> + underlying_type
> + underlying_value () const
> + {
> + return enum_value;
> + }
> +
> +public:
> + /* Allow default construction, just like raw enums. */
> + enum_flags ()
> + {}
Why not zero-initialize enum_value here? Given that enum_flags
represents a bitwise OR of enum_type, it seems reasonable that its
default value would be zero rather than uninitialzied.
> +
> + enum_flags (const enum_flags &other)
> + : enum_value (other.enum_value)
> + {}
> +
> + enum_flags &operator= (const enum_flags &other)
> + {
> + enum_value = other.enum_value;
> + return *this;
> + }
> +
> + /* If you get an error saying these two overloads are ambiguous,
> + then you tried to mix values of different enum types. */
> + enum_flags (enum_type e)
> + : enum_value (e)
> + {}
> + enum_flags (struct enum_flags::zero_type *zero)
> + : enum_value ((enum_type) 0)
> + {}
> +
> + enum_flags &operator&= (enum_type e)
> + {
> + enum_value = (enum_type) (underlying_value () & e);
> + return *this;
> + }
> + enum_flags &operator|= (enum_type e)
> + {
> + enum_value = (enum_type) (underlying_value () | e);
> + return *this;
> + }
> + enum_flags &operator^= (enum_type e)
> + {
> + enum_value = (enum_type) (underlying_value () ^ e);
> + return *this;
> + }
> +
> + operator enum_type () const
> + {
> + return enum_value;
> + }
> +
> + enum_flags operator& (enum_type e) const
> + {
> + return (enum_type) (underlying_value () & e);
> + }
> + enum_flags operator| (enum_type e) const
> + {
> + return (enum_type) (underlying_value () | e);
> + }
> + enum_flags operator^ (enum_type e) const
> + {
> + return (enum_type) (underlying_value () ^ e);
> + }
> +
> + enum_flags operator~ () const
> + {
> + return (enum_type) ~underlying_value ();
> + }
> +
> +private:
> + /* Stored as enum_type because GDB knows to print the bit flags
> + neatly if the enum values look like bit flags. */
> + enum_type enum_value;
> +};
> +
> +/* Global operator overloads. */
> +
> +template <typename enum_type>
> +typename enum_flags_type<enum_type>::type
> +operator| (enum_type e1, enum_type e2)
> +{
> + return enum_flags<enum_type> (e1) | e2;
> +}
> +
> +template <typename enum_type>
> +typename enum_flags_type<enum_type>::type
> +operator~ (enum_type e)
> +{
> + return ~enum_flags<enum_type> (e);
> +}
What are global operator^ and operator& omitted?