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

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/23/2021 1:47 PM, Martin Sebor via Gcc-patches wrote:
> On 6/22/21 5:28 PM, David Malcolm wrote:
>> 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...
>
> It's an argument to a a function, unlikely to confuse anyone.  But
> I also don't think it's important enough to argue about so I changed
> it along with the comment below.
>
>>>
>>> [...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?
>
> In my experience, names that differ only slightly (such as in just
> one letter) tend to easily lead to frustrating mistakes.  The new
> warning_suppressed_p() added in this patch also handles multiple
> warnings (via a wildcard) and uses a singular form, and as do
> the gimple_{get,set}no_warning() functions that are being replaced.
> So I don't think the suggested change is needed or would be
> an improvement.
>
> Similarly, there is no gimple_unset_no_warning() today and I don't
> think adding unsuppress_warning() is necessary or would add value,
> especially since there are just a handful of callers that re-enable
> warnings.  For the callers that might need to enable or disable
> a warning based on some condition, it's more convenient to do so
> in a single call then by having to call different functions based
> the value of the condition.
>
> In the attached patch I've changed the comment as you asked and
> also renamed the dis argument to supp but kept the function names
> the same.  I've also moved a few changes to tree.h from a later
> patch to this one to let it stand alone and regtested it on its
> own on x86_64-linux.  I'll plan to go ahead with this version
> unless requestsfor substantive changes come up in the review of
> the rest  of the changes.
>
> Thanks
> Martin
>
> gcc-no-warning-1.diff
>
> Add support for per-location warning groups.
>
> gcc/ChangeLog:
>
> 	* Makefile.in (OBJS-libcommon): Add diagnostic-spec.o.
> 	* gengtype.c (open_base_files): Add diagnostic-spec.h.
> 	* diagnostic-spec.c: New file.
> 	* diagnostic-spec.h: New file.
> 	* tree.h (no_warning, all_warnings, suppress_warning_at): New
> 	declarations.
> 	* warning-control.cc: New file.
>
> diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h
> new file mode 100644
> index 00000000000..62d9270ca6d
> --- /dev/null
> +++ b/gcc/diagnostic-spec.h
> @@ -0,0 +1,140 @@
> +/* Language-independent APIs to enable/disable per-location warnings.
> +
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   Contributed by Martin Sebor <msebor@redhat.com>
> +
> +   This file is part of GCC.
> +
> +   GCC is free software; you can redistribute it and/or modify it under
> +   the terms of the GNU General Public License as published by the Free
> +   Software Foundation; either version 3, or (at your option) any later
> +   version.
> +
> +   GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +   WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +   FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +   for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef DIAGNOSTIC_SPEC_H_INCLUDED
> +#define DIAGNOSTIC_SPEC_H_INCLUDED
> +
> +#include "hash-map.h"
> +
> +/* A "bitset" of warning groups.  */
> +
> +struct nowarn_spec_t
Should probably be "class" not "struct".  Though I don't think we're 
necessarily consistent with this.
> +private:
> +  /* Bitset of warning groups.  */
> +  unsigned bits;
Our conventions are to prefix member fields with m_.

Note that a simple "unsigned" may just be 32 bits.  Though with your 
grouping that may not matter in practice.

I think with the nits fixed this is fine.  I know there's some 
disagreement on singular vs plural, but having two names differing by 
just that is probably going to be more confusing in the end, so I'm on 
the side of using the names as-is.

jeff