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

[Mark Wielaard <mjw at redhat dot com>]

Hi Manuel,

On Sun, Sep 11, 2016 at 08:26:20PM +0100, Manuel López-Ibáñez 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?

Because they are different (subsets) of warnings, projects might prefer
warnings for a wider or smaller set. Also -Wshadow is known to have the
current behaviour, so changing it now will be confusing. Also google
already released a gcc variant with these warnings using these semantics.

> variables that will nevertheless trigger errors/warnings if used wrongly
> seems not very useful anyway.

I am sorry, I cannot follow your reasoning here.
Could you try to explain in different words or with a specific example?

> > +	    /* 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

Thanks. I think I fixed it in the attached. But note that this was
existing code and that the wiki page you reference doesn't mention
warning_at even once. If you want this kind of pattern changed in
the code base it would be helpful to add an example of how to change
existing code to use the new/correct way to the wiki so people can do
this as an easy hack.

> > +		  /* 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.

As I explained above I don't think changing the semantics of the warnings
is a good idea. There are projects currently using -Wshadow and expecting
it to warn as it does. I am not familiar with this "rant". If you believe
it is useful input to the discussion could you provide a pointer or ask
Linus to participate in this discussion to explain what his preferences
are?

> > +@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).

There are multiple testcases provided. I am not against enabling any of
these options in -Wall or -Wextra, but it would be good to see some
experiments using them first on some larger codebases (maybe gcc itself).
If you could try it on some and report what the rate of false positive
is (plus maybe some examples to show whether one might or might not want
to fix them anyway) then we can see if it makes sense. But I believe that
should be done independent from introducing the new warnings themselves.

Thanks,

Mark

Attached v2 of the patch with the suggested change above using warning_at.
No changes in the test results for any of the old or new -Wshadow tests.

Attachment: 0001-Add-Wshadow-local-and-Wshadow-compatible-local.patch
Description: Text document