On 5/27/21 5:19 AM, Richard Sandiford wrote:
Thanks for doing this.
Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
[…]
On 5/24/21 5:08 PM, David Malcolm wrote:
On Mon, 2021-05-24 at 16:02 -0600, Martin Sebor wrote:
Subsequent patches then replace invocations of the TREE_NO_WARNING()
macro and the gimple_no_warning_p() and gimple_set_no_warning()
functions throughout GCC with those and remove the legacy APIs to
keep them from being accidentally reintroduced along with the
problem.
These are mostly mechanical changes, except that most of the new
invocations also specify the option whose disposition to query for
the expression or location, or which to enable or disable[2].
The last function, copy_no_warning(), copies the disposition from
one expression or location to another.
A couple of design choices might be helpful to explain:
First, introducing "warning groups" rather than controlling each
individual warning is motivated by a) the fact that the latter
would make avoiding redundant warnings for related problems
cumbersome (e.g., after issuing a -Warray-bounds we want to
suppress -Wstringop-overflow as well -Wstringop-overread for
the same access and vice versa), and b) simplicity and efficiency
of the implementation (mapping each option would require a more
complex data structure like a bitmap).
Second, using location_t to associate expressions/statements with
the warning groups also turns out to be more useful in practice
than a direct association between a tree or gimple*, and also
simplifies managing the data structure. Trees and gimple* come
and go across passes, and maintaining a mapping for them that
accounts for the possibility of them being garbage-collected
and the same addresses reused is less than straightforward.
I find some of the terminology rather awkard due to it the negation
we're already using with TREE_NO_WARNING, in that we're turning on a
no_warning flag, and that this is a per-location/expr/stmt thing that's
different from the idea of enabling/disabling a specific warning
altogether (and the pragmas that control that). Sometimes the patches
refer to enabling/disabling warnings and I think I want "enabling" and
"disabling" to mean the thing the user does with -Wfoo and -Wno-foo.
Would calling it a "warning suppression" or somesuch be more or less
klunky?
I like warning suppression :) But I'm not sure where you suggest
I use the phrase.
I don't particularly care for the "no" in the API names either
(existing or new) and would prefer a positive form. I considered
enable_warning() and warning_enabled() but I chose the names I did
because they're close to their established gimple namesakes. I'm
fine changing them to the alternatives, or if you or someone else
has a preference for different names I'm open to suggestions. Let
me know.
Not my area, but FWIW, +1 for David's suggestion of
s/set_no_warning/suppress_warning/ or similar. As well as the
problem with the double negative in negated conditions, I always have to
remember whether TREE_NO_WARNING means that hasn't been anything to warn
about yet or whether we shouldn't warn in future.
Sure. Jason has a patch out for review to introduce
warning_enabled_at (location, int option).
With that, how does the following API look? (I'd replace the int
argument above with opt_code myself):
void enable_warning (location_t, enum opt_code = N_OPTS);
void disable_warning (location_t, enum opt_code = N_OPTS);
void copy_warning (location_t, location_t);
bool warning_enabled_at (location_t, enum opt_code = N_OPTS).
void enable_warning (tree, enum opt_code = N_OPTS);
void disable_warning (tree, enum opt_code = N_OPTS);
void copy_warning (tree, const_tree);
bool warning_enabled_at (const_tree, enum opt_code = N_OPTS).
void enable_warning (gimple *, enum opt_code = N_OPTS);
void disable_warning (gimple *, enum opt_code = N_OPTS);
void copy_warning (gimple *, const gimple *);
bool warning_enabled_at (gimple *, enum opt_code = N_OPTS).
Martin
Richard
