RFC: Enabling -Wshadow, by default?

[Friedrich W. H. Kossebau]

Hi,

TL;DR  what is people's take on warnings about variable shadowing, useful or 
going against naming habits? Would you prefer this being enabled by default 
for any projects, at least with clang, would you at least for your project?


I just stumbled over a (non-effective) bug due to variable shadowing in 
Okteta code, to discover that other than assumed by me by the default KDE 
settings the compilers are not instructed to warn about shadowing of 
variables. Checking with lxr.kde.org I only found some projects enable this 
warning flag, like kmymoney or kdiff3. Now while not recently, I remember 
that shadowing has been a category of issues causing bugs, especially on 
refactoring existing code, where name usages are recombined in new scopes.

Playing around and talking to waqar, who just happened to have resolved some 
shadowing issues in Kate (invent.kde.org/utilities/kate/-/merge_requests/
1971), it seems that while both gcc and clang support the -Wshadow flag, gcc 
is complaining a bit too much, like over shadowing of private members of 
base classes or around variables in lambdas vs. non-captured things (see 
snippet below for gcc bug report links).

Clang warnings though seemed mostly reasonable, so for Okteta I now add 
this, perhaps you might want to add this to your project as well:
--- 8< ---
if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
    # Not setting for GNU due to too many warnings related to private 
members of base classes or around lambdas
    # see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56556 or 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79328
    string(APPEND CMAKE_CXX_FLAGS " -Wshadow")
endif()
--- 8< ---

Now, fixing those warnings one finds there are some cases where the 
shadowing might not really be a critical issue. E.g. names of local helper 
variables which are later reused in another deeper scope, with little chance 
to get this wrong:
--- 8< ---
    [...]
    auto it = something();
    if (it) 
    [...]
    for (...) {
       [...]
       auto it = somethingelse();
       if (it)
       [...]
    }
--- 8< ---
In such cases it feels a bit annoying to have to invent a more complicated/
non-obvious name just to avoid the warning, or seeing to move the first case 
in a dedicated scope just for this sake.

Then, testing KCoreAddons, KConfig, KWidgetsAddons, & KXmlGui, with the 
warning flag results in quite some related warnings. So one would assume 
that quite some KDE projects would have a lot of places which trigger that 
warning, and thus suddenly the build log would get noisy, while people are 
currently not focused on this and no actual bugs might be uncovered by this.

So adding above snippet for everyone to KDECompilerSettings.cmake currently 
might not be something to consider. Instead people first might want to 
collect some experience with the flag when used on code across KDE projects? 
And only once a critical numbers of projects have code prepared to deal with 
that warning, one might switch this warning on by default.

What are your opinion on this?
And what are your related experiences and approaches, also outside of KDE, 
especially around code where the warning might conflict with non-risky 
naming habits?

Cheers
Friedrich