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

[Eric Gallager <egall at gwmail dot gwu dot edu>]

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.
>
> 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.


>> +	    /* If '-Wshadow-compatible-local' is specified without other
>> +	       -Wshadow flags, we will warn only when the types of the
>> +	       shadowing variable (i.e. new_decl) and the shadowed variable
>> +	       (old_decl) are compatible.  */
>> +	    if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl)))
>> +	      warning_code = OPT_Wshadow_compatible_local;
>> +	    else
>> +	      warning_code = OPT_Wshadow_local;
>> +	    warned = warning (warning_code,
>> +			      "declaration of %q+D shadows a parameter",
>> +			      new_decl);
>
> Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl).
> See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations
>
>
>> +		  /* If '-Wshadow-compatible-local' is specified without other
>> +		     -Wshadow flags, we will warn only when the type of the
>> +		     shadowing variable (i.e. x) can be converted to that of
>> +		     the shadowed parameter (oldlocal). The reason why we only
>> +		     check if x's type can be converted to oldlocal's type
>> +		     (but not the other way around) is because when users
>> +		     accidentally shadow a parameter, more than often they
>> +		     would use the variable thinking (mistakenly) it's still
>> +		     the parameter. It would be rare that users would use the
>> +		     variable in the place that expects the parameter but
>> +		     thinking it's a new decl.  */
>
> As said above, IMHO, this behavior should be the default of -Wshadow, or at
> least, of -Wshadow-local. The current behavior only leads to people not
> using -Wshadow (and us not including it in -Wall -Wextra). There is a Linus rant
> from some years ago that explains vehemently why Wshadow is useless in its
> current form.
>
>> +@item -Wshadow-compatible-local
>> +@opindex Wshadow-compatible-local
>> +@opindex Wno-shadow-compatible-local
>> +Warn when a local variable shadows another local variable or parameter
>> +whose type is compatible with that of the shadowing variable. In C++,
>> +type compatibility here means the type of the shadowing variable can be
>> +converted to that of the shadowed variable. The creation of this flag
>> +(in addition to @option{-Wshadow-local}) is based on the idea that when
>> +a local variable shadows another one of incompatible type, it is most
>> +likely intentional, not a bug or typo, as shown in the following
>> example:
>
> -Wshadow-compatible-local seems safe enough to be enabled by -Wall (or
> -Wextra). Options not enabled by either are rarely used (and, hence, rarely
> tested).
>
> Cheers,
>
> Manuel.
>


I see lots of GNU projects at least using the gnulib manywarnings.m4
autoconf macros in their configure script, and that enables a lot more
warnings than just -Wall and -Wextra. So I think the test coverage for
warnings outside of -Wall or -Wextra is better than you might think.