On Tue, 2021-06-22 at 19:18 -0400, David Malcolm wrote:
> On Fri, 2021-06-04 at 15:41 -0600, Martin Sebor wrote:
> > The attached patch introduces the suppress_warning(),
> > warning_suppressed(), and copy_no_warning() APIs without making
> > use of them in the rest of GCC. They are in three files:
> >
> > diagnostic-spec.{h,c}: Location-centric overloads.
> > warning-control.cc: Tree- and gimple*-centric overloads.
> >
> > The location-centric overloads are suitable to use from the
> > diagnostic
> > subsystem. The rest can be used from the front ends and the middle
> > end.
>
> [...snip...]
>
> > +/* Return true if warning OPT is suppressed for decl/expression
> > EXPR.
> > + By default tests the disposition for any warning. */
> > +
> > +bool
> > +warning_suppressed_p (const_tree expr, opt_code opt /* =
> > all_warnings */)
> > +{
> > + const nowarn_spec_t *spec = get_nowarn_spec (expr);
> > +
> > + if (!spec)
> > + return get_no_warning_bit (expr);
> > +
> > + const nowarn_spec_t optspec (opt);
> > + bool dis = *spec & optspec;
> > + gcc_assert (get_no_warning_bit (expr) || !dis);
> > + return dis;
>
> Looking through the patch, I don't like the use of "dis" for the "is
> suppressed" bool, since...
>
> [...snip...]
>
> > +
> > +/* Enable, or by default disable, a warning for the statement STMT.
> > + The wildcard OPT of -1 controls all warnings. */
>
> ...I find the above comment to be confusingly worded due to the double-
> negative.
>
> If I'm reading your intent correctly, how about this wording:
>
> /* Change the supression of warnings for statement STMT.
> OPT controls which warnings are affected.
> The wildcard OPT of -1 controls all warnings.
> If SUPP is true (the default), enable the suppression of the
> warnings.
> If SUPP is false, disable the suppression of the warnings. */
>
> ...and rename "dis" to "supp".
>
> Or have I misread the intent of the patch?
>
> > +
> > +void
> > +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
> > + bool dis /* = true */)
>
> > +{
> > + if (opt == no_warning)
> > + return;
> > +
> > + const key_type_t key = convert_to_key (stmt);
> > +
> > + dis = suppress_warning_at (key, opt, dis) || dis;
> > + set_no_warning_bit (stmt, dis);
> > +}
>
> [...snip...]
Also, I think I prefer having a separate entrypoints for the "all
warnings" case; on reading through the various patches I think that in
e.g.:
- TREE_NO_WARNING (*expr_p) = 1;
+ suppress_warning (*expr_p);
I prefer:
suppress_warnings (*expr_p);
(note the plural) since that way we can grep for them, and it seems
like better grammar to me.
Both entrypoints could be implemented by a static suppress_warning_1
internally if that makes it easier.
In that vein, "unsuppress_warning" seems far clearer to me that
"suppress_warning (FOO, false)"; IIRC there are very few uses of this
non-default arg (I couldn't find any in a quick look through the v2
kit).
Does this make sense?
Dave
