Re: [PATCH 2/3] Use type_instance_flags more throughout

Pedro Alves
2020-09-14T13:53:11Z

On 8/26/20 4:21 PM, Andrew Burgess wrote:

>> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
>> index 55a6dafb7e..b42cef6137 100644
>> --- a/gdb/gdbtypes.h
>> +++ b/gdb/gdbtypes.h
>> @@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
>> TYPE_ZALLOC (type, \
>> sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
>>
>> -#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
>> +#define TYPE_INSTANCE_FLAGS(thistype) \
>> + type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
>> +#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
>> + (thistype)->instance_flags = flags
>
> There are some places where you've not updated a use of
> TYPE_INSTANCE_FLAGS. The patch below fixes these. In order to find
> these I changed TYPE_INSTANCE_FLAGS to make use of a member function
> returning a 'const type_instance_flags'.
>
> Feel free to take any parts of this patch you think are useful.

Wow, these shouldn't even compile. That is showing a hole in enum_flags.

>
> - TYPE_INSTANCE_FLAGS (builtin_d_type->builtin_byte)
> - |= TYPE_INSTANCE_FLAG_NOTTEXT;

TYPE_INSTANCE_FLAGS does a cast, so this is the equivalent of:

int flags = 0;
(int) flags |= 1;

which of course doesn't compile:

test.c:1:18: error: lvalue required as left operand of assignment
1 | (int) flags |= 1;
| ^

I'm merging the patch below into the main enum flags patch, to
disable the rvalue versions of compound assignment. It catches the
spots you caught as well:

src/gdb/d-lang.c: In function ‘void* build_d_types(gdbarch*)’:
src/gdb/d-lang.c:325:8: error: use of deleted function ‘void enum_flags<E>::operator|=(enum_flags<E>) && [with E = type_instance_flag_value]’
325 | |= TYPE_INSTANCE_FLAG_NOTTEXT;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~

src/gdb/ft32-tdep.c: In function ‘gdbarch* ft32_gdbarch_init(gdbarch_info, gdbarch_list*)’:
src/gdb/ft32-tdep.c:580:42: error: use of deleted function ‘void enum_flags<E>::operator|=(enum_flags<E>) && [with E = type_instance_flag_value]’
580 | TYPE_INSTANCE_FLAGS (tdep->pc_type) |= TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'll merge your changes into the type_instance_flags patch.

From aadb29540a43d96dc234d0d8126e5081926eec02 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 14 Sep 2020 14:48:34 +0100
Subject: [PATCH] delete rval

---
gdb/unittests/enum-flags-selftests.c | 18 +++++++++---------
gdbsupport/enum-flags.h | 18 +++++++++++-------
2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/gdb/unittests/enum-flags-selftests.c b/gdb/unittests/enum-flags-selftests.c
index 17ab5c9b09..af585f04ae 100644
--- a/gdb/unittests/enum-flags-selftests.c
+++ b/gdb/unittests/enum-flags-selftests.c
@@ -174,9 +174,9 @@ CHECK_VALID (true, EF, EF () ^ RE ())
CHECK_VALID (false, void, RE () |= RE2 ())
CHECK_VALID (false, void, RE () &= RE2 ())
CHECK_VALID (false, void, RE () ^= RE2 ())
-CHECK_VALID (true, RE&, RE () |= RE ())
-CHECK_VALID (true, RE&, RE () &= RE ())
-CHECK_VALID (true, RE&, RE () ^= RE ())
+CHECK_VALID (false, void, RE () |= RE ())
+CHECK_VALID (false, void, RE () &= RE ())
+CHECK_VALID (false, void, RE () ^= RE ())

/* operator OP= (raw_enum, raw_enum), lvalue ref on the lhs. */

@@ -192,9 +192,9 @@ CHECK_VALID (true, RE&, re ^= RE ())
CHECK_VALID (false, void, EF () |= RE2 ())
CHECK_VALID (false, void, EF () &= RE2 ())
CHECK_VALID (false, void, EF () ^= RE2 ())
-CHECK_VALID (true, EF&, EF () |= RE ())
-CHECK_VALID (true, EF&, EF () &= RE ())
-CHECK_VALID (true, EF&, EF () ^= RE ())
+CHECK_VALID (false, void, EF () |= RE ())
+CHECK_VALID (false, void, EF () &= RE ())
+CHECK_VALID (false, void, EF () ^= RE ())

/* operator OP= (enum_flags, raw_enum), lvalue ref on the lhs. */

@@ -210,9 +210,9 @@ CHECK_VALID (true, EF&, ef ^= EF ())
CHECK_VALID (false, void, EF () |= EF2 ())
CHECK_VALID (false, void, EF () &= EF2 ())
CHECK_VALID (false, void, EF () ^= EF2 ())
-CHECK_VALID (true, EF&, EF () |= EF ())
-CHECK_VALID (true, EF&, EF () &= EF ())
-CHECK_VALID (true, EF&, EF () ^= EF ())
+CHECK_VALID (false, void, EF () |= EF ())
+CHECK_VALID (false, void, EF () &= EF ())
+CHECK_VALID (false, void, EF () ^= EF ())

/* operator OP= (enum_flags, enum_flags), lvalue ref on the lhs. */

diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index b3e317ecb9..d30d90b68d 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -147,22 +147,27 @@ class enum_flags
: m_enum_value ((enum_type) 0)
{}

- enum_flags &operator&= (enum_flags e)
+ enum_flags &operator&= (enum_flags e) &
{
m_enum_value = (enum_type) (m_enum_value & e.m_enum_value);
return *this;
}
- enum_flags &operator|= (enum_flags e)
+ enum_flags &operator|= (enum_flags e) &
{
m_enum_value = (enum_type) (m_enum_value | e.m_enum_value);
return *this;
}
- enum_flags &operator^= (enum_flags e)
+ enum_flags &operator^= (enum_flags e) &
{
m_enum_value = (enum_type) (m_enum_value ^ e.m_enum_value);
return *this;
}

+ /* Delete rval versions. */
+ void operator&= (enum_flags e) && = delete;
+ void operator|= (enum_flags e) && = delete;
+ void operator^= (enum_flags e) && = delete;
+
/* Like raw enums, allow conversion to the underlying type. */
constexpr operator underlying_type () const
{
@@ -278,9 +283,8 @@ using is_enum_flags_enum_type_t
/* rval reference version. */ \
template <typename enum_type, \
typename = is_enum_flags_enum_type_t<enum_type>> \
- constexpr enum_type & \
- OPERATOR_OP (enum_type &&e1, enum_type e2) \
- { return e1 = e1 OP e2; } \
+ void \
+ OPERATOR_OP (enum_type &&e1, enum_type e2) = delete; \
\
/* Delete compound assignment from unrelated types. */ \
\
@@ -291,7 +295,7 @@ using is_enum_flags_enum_type_t
\
template <typename enum_type, typename other_enum_type, \
typename = is_enum_flags_enum_type_t<enum_type>> \
- constexpr enum_type & \
+ void \
OPERATOR_OP (enum_type &&e1, other_enum_type e2) = delete;

ENUM_FLAGS_GEN_BINOP (operator|, |)

--
2.14.5