build: Disallow shadowing variables via -isystem, -Wshadow #15377

1. 
   Empact commented at 11:46 pm on February 9, 2019: member

   Shadowed variables are implicated in logic errors, duplicative code, and unnecessary operation. Catching them automatically is best.

   As with -Wdocumentation, warns when configured with –enable-isystem, errors if configured with –enable-werror as well.

   This is built on / depends on: #14920

2. build: Optionally include dependency headers with -isystem

   When configured with --enable-isystem.
   
   Was necessary to split QT_INCLUDES into QT_INCLUDES and
   QT_MOC_INCLUDES because moc does not understand -isystem, e.g.:
   
       Unknown options: isystem/usr/local/Cellar/qt/5.10.0_1/include/QtNetwork[...]
   
   This does not convert all uses, but focuses on libraries which have triggered
   warnings/errors when applying initial additional build checks: QT, Univalue, and Berkeley DB.
   LevelDb requires additional measures as its code is compiled with the project warnings
   via AM_CXXFLAGS.
   
   Note -isystem should not be applied to /usr/include, see BITCOIN_SYSTEM_INCLUDE
   for a helper to convert -I to -isystem with /usr/include excepted.

   07e7cccba1
3. build: Enable -Wdocumentation if isystem is enabled

   -Werror=documentation if isystem & werror are enabled.

   a98e0be593
4. Empact renamed this:
   Disallow shadowing variables via -isystem, -Wshadow
   build: Disallow shadowing variables via -isystem, -Wshadow
   on Feb 9, 2019
5. fanquake added the label Build system on Feb 9, 2019
6. 
   DrahtBot commented at 0:41 am on February 10, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
   • #14641 (rpc: Add min_conf option to fund transaction calls by promag)
   • #14137 (gui: Add Windows taskbar progress by ken2812221)
   • #13728 (WIP: Scripts and tools: Run the CI lint stage on mac by Empact)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

7. Empact force-pushed on Feb 10, 2019
8. 
   practicalswift commented at 11:43 am on February 10, 2019: contributor

   Concept ACK
9. 
   laanwj commented at 3:50 pm on February 10, 2019: member

   Sorry, but NACK. Please not this again. I still remember what a clusterfck enabling shadowing warnings was last time. It created such a torrend of PRs:

   #9911 #10009 #10010 …

   It also wasn’t consistent between compilers so just that you have no shadowing warnings doesn’t mean that no one else has, triggering a ‘shadowing fixup’ PR after every other normal PR, until it was finally disabled in #10136.

10. 
    practicalswift commented at 4:03 pm on February 10, 2019: contributor

    @laanwj What about only enabling in Travis for clang? That way there will be no need for manual reviewers pointing out accidental shadowing: that will already have been fixed before manual review starts. I see quite a few of these when doing review work.
11. 
    Empact commented at 7:21 pm on February 10, 2019: member

    Yeah, to distinguish this from those other PRs, with this Travis will fail on the introduction of a violation, so dealing with them will be off-loaded from the reviewer to the pull requester, based on that this should reduce review burden.
12. Disallow shadowing variables via -isystem, -Wshadow

    Shadowed variables are implicated in logic errors, duplicative code, and
    unnecessary operation. Catching them automatically is best.
    
    As with -Wdocumentation, warns when configured with --enable-isystem,
    errors if configured with --enable-werror as well.

    af8d17be05
13. Empact force-pushed on Feb 11, 2019
14. 
    gmaxwell commented at 8:29 pm on February 11, 2019: contributor

    NAK. Forcing contributors to do battle with the random failures of a remote compiler is not attractive.

    I wish shadow warnings were useful in C++ like they are in C, but they aren’t. They’re continually spurious. We tried shadow warnings before and it was a nuisance. I don’t expect this to change unless compiler authors add an additional kind of narrower scope shadow warning.

    Furthermore, I’m not aware of any bugs in this project being introduced or hidden by shadowing so far. I am, however, aware of PRs that created bugs trying to silence warnings.

15. 
    Empact commented at 8:51 pm on February 11, 2019: member

    Closing due to controversy
16. Empact closed this on Feb 11, 2019

    ---

17. DrahtBot locked this on Dec 16, 2021

Contributors
Empact DrahtBot practicalswift laanwj gmaxwell

Labels
Build system