build: Disallow shadowing variables via -isystem, -Wshadow #15377
Conversation
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.
-Werror=documentation if isystem & werror are enabled.
Empact
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
|
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: 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. |
|
@laanwj What about only enabling in Travis for |
|
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. |
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.
|
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. |
|
Closing due to controversy |
6 participants
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