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