-
Notifications
You must be signed in to change notification settings - Fork 38.8k
build: enable -Wdocumentation #21613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Concept ACK |
|
Nice! I was thinking about a doxygen linter at some point but happy this is part of C++ compilers now. Concept and code review ACK a4e970a |
|
cr ACK a4e970a: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback |
|
Approach ACK and nice fixes that this finds. This is handy indeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light ACK a4e970a skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted
In file included from test/addrman_tests.cpp:10:
./netbase.h:237:11: warning: parameter 'sock' not found in the function declaration [-Wdocumentation]
* @param sock The SOCKS5 proxy socket.
^~~~
./netbase.h:237:11: note: did you mean 'socket'?
* @param sock The SOCKS5 proxy socket.
^~~~
socket
1 warning generated.
a4e970a build: enable -Wdocumentation if suppressing external warnings (fanquake) 3b0078f doc: fixup -Wdocumentation issues (fanquake) c6edcf1 build: suppress libevent warnings if supressing external warnings (fanquake) Pull request description: Enable `-Wdocumentation` by taking advantage of our `--enable-suppress-external-warnings` flag. Most of the CIs are using this flag now, so any regressions should be caught. This also required modifying libevents flags when suppressing warnings, as depending on the version being built against, that could generate a large number of warnings. i.e: ```bash In file included from httpserver.cpp:34: In file included from ./support/events.h:12: /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:464:11: warning: parameter 'req' not found in the function declaration [-Wdocumentation] @param req a request object ^~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:465:11: warning: parameter 'databuf' not found in the function declaration [-Wdocumentation] @param databuf the data chunk to send as part of the reply. ^~~~~~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:467:11: warning: parameter 'call' not found in the function declaration [-Wdocumentation] @param call back's argument. ^~~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:939:4: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync] @deprecated This function is deprecated; you probably want to use ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:946:1: note: add a deprecation attribute to the declaration to silence this warning char *evhttp_decode_uri(const char *uri); ^ __AVAILABILITY_INTERNAL_DEPRECATED /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:979:5: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync] @deprecated This function is deprecated as of Libevent 2.0.9. Use ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:987:1: note: add a deprecation attribute to the declaration to silence this warning int evhttp_parse_query(const char *uri, struct evkeyvalq *headers); ^ __AVAILABILITY_INTERNAL_DEPRECATED /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: warning: parameter 'query_parse' not found in the function declaration [-Wdocumentation] @param query_parse the query portion of the URI ^~~~~~~~~~~ /usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: note: did you mean 'uri'? @param query_parse the query portion of the URI ^~~~~~~~~~~ uri 69 warnings generated. ``` Note that a lot of these have already been fixed upstream. ACKs for top commit: laanwj: Concept and code review ACK a4e970a practicalswift: cr ACK a4e970a: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback jonatack: Light ACK a4e970a skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted Tree-SHA512: 57a1e30cffcc8bcceee72d85f58ebe29eae525861c70acb237541bd480c51ede89875c033042c0af376fdbb49fb7f588ef9282a47c6e78f9d4501c41f1b21eb6
|
Approach ACK I think the new flags |
We can't do this for the reasons outlined in the PR description. Anyone building with versions of upstream libraries where all |
Summary: This fixes documentation errors, such as wrong parameter names, empty documentation for a parameter, documentation not located next to the targetted function. This is a backport of [[ bitcoin/bitcoin#21613 | core#21613 ]] Test Plan: ``` cd build ccache --clear rm -Rf * cmake .. -Ninja -DCMAKE_CXX_COMPILER=clang++ ninja ninja doc-doxygen ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11783
Enable
-Wdocumentationby taking advantage of our--enable-suppress-external-warningsflag. Most of the CIs are using this flag now, so any regressions should be caught.This also required modifying libevents flags when suppressing warnings, as depending on the version being built against, that could generate a large number of warnings. i.e:
Note that a lot of these have already been fixed upstream.