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