Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.

[Jason Merrill <jason at redhat dot com>]

On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering <jim@meyering.net> wrote:
> On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>> On 9/11/16, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
>>> On 11/09/16 14:02, Mark Wielaard wrote:
>>>>   -Wshadow-local which warns if a local variable shadows another local
>>>>   variable or parameter,
>>>>
>>>>   -Wshadow-compatible-local which warns if a local variable shadows
>>>>   another local variable or parameter whose type is compatible with that
>>>>   of the shadowing variable.
>
> Hi Mark,
> Thank you for doing this!
>
>>> I honestly don't see the need for the second flag. Why not make Wshadow, or at
>>> least Wshadow-local, work in this way by default? Warning for shadowed
>>> variables that will nevertheless trigger errors/warnings if used wrongly
>>> seems not very useful anyway.
>>
>> It helps for readability and helps pre-emptively catch those other
>> errors/warnings that come from using them wrongly before they occur in
>> a more confusing manner. Plus more granularity makes it easier to
>> manage the workload of warning-silencing.
>
> The new warnings will be especially welcome in C++ code.
> -Wshadow is fine for most C code, but in C++ it is very common to have
> method names that shadow class data member names and/or local
> variables in any member function definition. Thus, using -Wshadow in
> C++ code often forces undesirable (and unscalable) renaming to avoid
> such shadowing, while the only important type of shadowing is that
> caught by the new options. Many of us want the benefit of the new
> options (spotting bad, easily-fixed code), without having to incur the
> renaming (often hurting readability/maintainability) required to
> enable -Werror=shadow.

I wonder about spelling the options as
-Wshadow={local,compatible-local} rather than with a dash, but
otherwise the patch looks fine.

Jason