Re: [PATCH 0/11] warning control by group and location (PR 74765)

[Jason Merrill <jason@redhat.com>]

On 5/27/21 5:55 PM, David Malcolm wrote:
> On Thu, 2021-05-27 at 10:41 -0600, Martin Sebor wrote:
>> 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).
> 
> CCing Jason.
> 
> BTW, do you have a URL for that patch?

https://gcc.gnu.org/pipermail/gcc-patches/2021-May/571307.html

is the most recent version.

Jason