On 9/14/16, Jason Merrill <ja...@redhat.com> wrote:
> On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering <j...@meyering.net> wrote:
>> On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager <eg...@gwmail.gwu.edu>
>> wrote:
>>> On 9/11/16, Manuel López-Ibáñez <lopeziba...@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
>
What would the current behavior be called? Maybe
-Wshadow={all,local,compatible-local} instead? Also remember -Werror:
with another '=' you could end up with 2 of them with something like
-Werror=shadow=local which looks more confusing than the hyphenated
version.
Eric
