This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Rs6000 infrastructure cleanup (switches), revised patch #4
On Mon, 15 Oct 2012, Michael Meissner wrote:
> > I think changing the names is safer - it's immediately obvious as a build
> > failure if you missed anything. If you have MASK_* names for bits in more
> > than one flags variable, there's a risk of accidentally testing a bit in
> > the wrong variable, or ORing together bits that belong in different
> > variables in a way that can't possibly work, without this causing
> > immediately visible problems. Maybe you're actually only using the names
> > for a single variable, but it still seems error-prone for future changes.
>
> Well to be safest, we should have a prefix for each word if you define more
> than one flag word. Preferably a name that the user can specify in the .opt
> file.
Yes, for MASK_*.
> > I guess TARGET_* names should be safe in a way that MASK_* ones aren't for
> > multiple variables - but then I wouldn't have options to do things two
> > different ways, but instead use TARGET_* instead of OPTION_* and fix
> > existing uses of OPTION_* for such bits.
>
> I can see the MASK/OPTION_MASK thing, but not having TARGET_* means there are
> lots and lots of code changes.
>
> Unfortunately in order to bring the number of changes down to a point where the
> patches can be reviewed, my previous patches did:
>
> #define TARGET_FOO OPTION_FOO
> #define MASK_FOO OPTION_MASK_FOO
The first of those #defines might be an intermediate step towards actually
replacing OPTION_FOO by TARGET_FOO everywhere (since there seems to be no
actual need for the different naming convention there, only for the
masks). But I don't really think we should delay the mechanical
replacement much (changing all OPTION_* that aren't OPTION_MASK_* to be
TARGET_* should be a straightforward change to make automatically,
although a pain to review the results of that substitution so maybe best
kept in a separate patch from one doing anything more substantive).
That is:
1. Patch adding TARGET_FOO aliases for OPTION_FOO (small change to the awk
scripts and associated documentation, I expect).
2. Large, mechanical, automatically generated patch to change existing
OPTION_FOO users (or maybe one such patch per target).
3. Patch removing the OPTION_FOO name (small change to awk scripts and
documentation).
Then you've eliminated one unnecessary cause of changes when moving bits
out of target_flags.
> If TargetName were defined, it would use TARGET_<xxx> instead of OPTION_<xxx>,
> but the OPTION_MASK_<xxx> would not be changed.
Not needed, given the above sequence of changes.
> If SetFunction was defined, the opt*.awk files would generate:
>
> #define SET_FOO(VALUE) \
> do { \
> if (VALUE) \
> target_flags &= ~MASK_FOO; \
> else \
> target_flags |= MASK_FOO; \
> } while (0)
>
> If ExplicitFunction was defined, the opt*.awk files would generate:
>
> #define EXPLICIT_FOO(VALUE) \
> ((global_options_set.x_target_flags & MASK_FOO) != 0)
I'd like any such new macros to take an argument that's the pointer to the
relevant options structure (global_options, global_options_set). If the
place where the macro is called has a pointer available, then it can be
passed in, otherwise pass in &global_options or &global_options_set unless
and until such a pointer becomes available in the relevant place.
> How would you feel about SetFunction, ExplicitFunction, and the reduced
> TargetName?
The principle of having macros for setting flags or testing if they are
explicitly set is fine, though it's not clear to me that they need any
such special settings as SetFunction and ExplicitFunction (rather than
being generated unconditionally).
I'd quite like the macros such as target_flags that refer to global
options to end up not being lvalues at all. That helps ensure that option
settings are only modified in limited places that have options pointers.
It would be nice eventually for such things as "optimize" and "target"
attributes to be able to swap options structures, and to work closer to
how options on the command line are processed - for that, you want careful
control on what places actually modify options at all.
--
Joseph S. Myers
joseph@codesourcery.com