Re: [PATCH] Rs6000 infrastructure cleanup (switches), question

[David Edelsohn <dje dot gcc at gmail dot com>]

On Tue, Oct 9, 2012 at 7:37 PM, Michael Meissner
<meissner@linux.vnet.ibm.com> wrote:
> No before I go an redo the main part of patch #2, I have a question, which
> people prefer.
>
> The current code has sequences of:
>
>     target_flags |= MASK_FOO;                   /* set -mfoo */
>     if ((target_flags_explicit & MASK_FOO)) ... /* Did user do -mfoo or
>                                                     -mno-foo */
>
> I can either change the code like I did before:
>
>     rs6000_isa_flags |= OPTION_MASK_FOO;
>     if ((rs6000_isa_flags_explicit & OPTION_MASK_FOO)) ...
>
> Or I can add various macros to 'hide' the underlying bit manipulation, which
> would allow me to stage all of the changes into multiple patches.  I tend to
> prefer the raw bit manipulation because it is less change overall, we've used
> raw bit manipulation forever.  However I'm willing to add the accessor macros
> and submit multiple patches to get this checked in:
>
>    #define TARGET_FOO_EXPLICIT ((target_flags_explict & MASK_FOO) != 0)
>
>    #define SET_TARGET_FOO(VALUE)                                        \
>    do {                                                                 \
>      if (VALUE)                                                         \
>        target_flags |= MASK_FOO;                                        \
>      else                                                               \
>        target_flags &= ~MASK_FOO;                                       \
>    } while (0)
>
>    #define SET_TARGET_FOO_EXPLICIT (target_flags_explicit |= MASK_FOO)
>
> and then once all of the raw access to target_flags and target_flags_explicit
> is done via accessors, I can finally do the change:
>
>    #define TARGET_FOO_EXPLICIT                                          \
>      ((rs6000_isa_flags_explict & OPTION_MASK_FOO) != 0)
>
>    #define SET_TARGET_FOO(VALUE)                                        \
>    do {                                                                 \
>      if (VALUE)                                                         \
>        rs6000_isa_flags |= OPTION_MASK_FOO;                             \
>      else                                                               \
>        rs6000_isa_flags &= ~OPTION_MASK_FOO;                            \
>    } while (0)
>
>    #define SET_TARGET_FOO_EXPLICIT                                      \
>      (rs6000_isa_flags_explicit |= OPTION_MASK_FOO)
>
> An alternative would be to provide separate SET/CLEAR macros:
>
>    #define TARGET_FOO_EXPLICIT ((target_flags_explict & MASK_FOO) != 0)
>
>    #define SET_TARGET_FOO (target_flags |= MASK_FOO)
>    #define CLEAR_TARGET_FOO (target_flags &= ~MASK_FOO)
>
>    #define SET_TARGET_FOO_EXPLICIT (target_flags_explicit |= MASK_FOO)

Let's stick with the bit manipulation for the next step.  I think it
would be cleaner to switch to accessor macros, but that will make this
patch bigger and more complicated.  I don't think that switching piece
by piece helps any more.  We can add macros in another step.

Thanks, David