Re: [PATCH 1/8] gdbsupport: Provide global operators |=, &=, and ^= for enum bit flags

[Andrew Burgess <andrew.burgess@embecosm.com>]

Tom,

I think I gave up on this patch too easily.  I'd like to push back and
argue in favour of this change.

* Tom Tromey <tom@tromey.com> [2020-08-15 11:16:46 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The current enum bit flags mechanism provides global operators &, |,
> Andrew> ^, ~, but does not provide &=, |=, ^=.
> 
> Andrew> The implementation for one of these, |=, would look like this:
> 
> Andrew>   template <typename enum_type>
> Andrew>   typename enum_flags_type<enum_type>::type &
> Andrew>   operator|= (enum_type &e1, enum_type e2)
> 
> Why "enum_type &e1" and not "enum_flags_type<enum_type>::type &e1"
> here?

Unless I'm really messing up here, then when I try this I get the
compiler error:

  enum-flags.h:217:41: error: declaration of ‘operator|=’ as non-function
    217 | operator|= (enum_flags_type<enum_type>::type &e1, enum_type e2)

Though I can't understand what the error is trying to tell me, I can
understand why the compiler is unhappy.  When you consider the
enum_flags operator|= and the global operator|= they both now have the
exact same function signature, so there's no way for the compiler to
pick between them.

> 
> Andrew>   DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags);
> 
> Andrew>   enum some_flag f = flag_val1 | flag_val2;
> Andrew>   f |= flag_val3;
> 
> To me this example looks incorrect -- the idea behind enum flags is to
> not use the "enum some_flag" type, but instead the wrapper.  So it
> should be:
> 
>     some_flags f = flag_val1 | flag_val2;
>     f |= flag_val3;
> 
> Could you say why you want to use the enum type instead?  That would
> help me understand this patch.

I agree, but...

My argument would be:

 1. By making the change I propose we loose nothing, but my example
 above will just work, so we do gain something.

 2. We already have some global operators, these are only used when we
 run into cases like my original example, so clearly the code was
 originally written (I'm assuming) with the idea of supporting both
 use cases.

 3. Instead of writing 'enum some_flag', a developer might just write
 'some_flag'.  In this case they still require the changes from my
 patch if they ever want to use operator|=.  I think spotting the
 missing 's' is much harder during review, so it's easy for uses of
 'some_flag' to creep into the code base, then a future developer
 wanting to use 'operator|=' will need to fix up the 'some_flag' to
 'some_flags' miss-match.  Though it's easy to argue that the first
 developer made a mistake, and we frequently have to fix the mistakes
 of those going before, in this case we don't have to, so why force
 the matter?

I guess my argument would be, lets commit.  We should either remove
the existing global operators from enum-flags.h, fix up the fall out,
and so make it much harder to developers to use the 'some_flag'
version (so forcing the use of 'some_flags'), or make the change I
propose which allows the full range of operators while loosing non of
the protection that already exists.

Thanks,
Andrew