Re: [PATCH 1/13] v2 [PATCH 1/13] Add support for per-location warning groups (PR 74765)

[David Malcolm <dmalcolm@redhat.com>]

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