Build: enable -Wdocumentation via isystem#14920
Build: enable -Wdocumentation via isystem#14920Empact wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
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 Thanks for working on this! |
|
I think this can be useful for warnings. Which compilers/versions is |
|
For GCC, it has been available since 1994, the egcs days. gcc-mirror/gcc@0330c07 For Clang, has been in the codebase since at least 2009, coinciding with 2.7, though it wasn't mentioned in the release notes I checked. llvm-mirror/clang@33a33d8 MSVC has had a version of this since 2017, but AFAIK this should not affect those builds. https://blogs.msdn.microsoft.com/vcblog/2017/12/13/broken-warnings-theory/ |
|
Another option is |
|
Concept ACK |
TIL! Thanks for checking. That's far back enough, given that the minimum releases supported according to
Right—MSVC is a different beast. |
|
utACK—would be nice if @theuni could take a look at the build system changes. |
Empact
force-pushed
the
isystem
branch
3 times, most recently
from
8d363ce to
0ecc2db
Compare
|
I dropped the leveldb changes as leveldb is built with the warning options via |
Empact
force-pushed
the
isystem
branch
3 times, most recently
from
3f8a5d3 to
6ebba66
Compare
Empact
force-pushed
the
isystem
branch
2 times, most recently
from
9b50998 to
d768733
Compare
|
Looks like this has issues on two of the Travis linux builds (re-ran to make sure). |
Empact
force-pushed
the
isystem
branch
5 times, most recently
from
d508cfe to
cd87913
Compare
|
Thanks, the redirection issue was a dash incompatibility, and the boost issue was a case of This is ready for review. |
Empact
force-pushed
the
isystem
branch
2 times, most recently
from
dae3db6 to
72878aa
Compare
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.
Or -Werror=documentation if isystem & werror are enabled.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
297e098 Fix doxygen errors (Ben Woosley) Pull request description: These are all the remaining errors identified via -Werror=documentation, e.g.: ``` ./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation] * @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain ^~~~~~~ ./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'? * @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain ^~~~~~~ prevTxsUnival netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation] * @param outProxyConnectionFailed[out] Whether or not the connection to the ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'? * @param outProxyConnectionFailed[out] Whether or not the connection to the ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ outProxyConnectionFailed ``` You can use this to run with `-Wdocumentation` yourself: bitcoin#14920 ACKs for top commit: laanwj: ACK 297e098 Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
|
Thanks for the contributions here, however I'm going to enable |
9 participants
This converts some dependency includes to use -isystem, so that we can
enable additional compiler checks and warnings independent of
the conformance of those dependencies.
The included new check is -Wdocumentation, which was previously proposed under #13914. See #14103 for examples of issues this has identified.
Here's a significant list of other clang checks we could consider given this. Some additional checks would require additional -isystem conversions, e.g. LevelDB is not converted here:
-Wreserved-id-macro
-Wthread-safety-attributes #15556
-Wthread-safety-negative
-Wfloat-equal
-Wfloat-conversion
-Wsign-conversion
-Wstring-conversion
-Wcast-qual
-Wcast-align
-Wshorten-64-to-32
-Wconversion
-Wfloat-conversion
-Wdouble-promotion
-Wshadow #15377-Wshadow-field-in-constructor
-Wunused-exception-parameter
-Wunreachable-code-break
-Wdocumentation-unknown-command
-Wgnu-zero-variadic-macro-arguments
-Wgnu-anonymous-struct
-Wglobal-constructors
-Wexit-time-destructors
-Wnon-virtual-dtor
-Wweak-vtables
-Wcomma
-Wmissing-variable-declarations
-Wundefined-func-template
-Wzero-as-null-pointer-constant #15112
-Winconsistent-missing-destructor-override
-Wswitch-enum
-Wcovered-switch-default
-Wmissing-noreturn
-Wextra-semi
-Wc++17-extensions
-Wpadded
-Wold-style-cast
-Wmissing-prototypes