build: enable -Wdocumentation

fanquake commented at 6:56 am on April 6, 2021: member

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:

 0In file included from httpserver.cpp:34:
 1In file included from ./support/events.h:12:
 2/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:464:11: warning: parameter 'req' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) req a request object
 3          ^~~
 4/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:465:11: warning: parameter 'databuf' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) databuf the data chunk to send as part of the reply.
 5          ^~~~~~~
 6/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:467:11: warning: parameter 'call' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) call back's argument.
 7          ^~~~
 8/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](/bitcoin-bitcoin/contributor/deprecated/)  This function is deprecated; you probably want to use
 9  ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10/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
11char *evhttp_decode_uri(const char *uri);
12^
13__AVAILABILITY_INTERNAL_DEPRECATED 
14/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](/bitcoin-bitcoin/contributor/deprecated/) This function is deprecated as of Libevent 2.0.9.  Use
15   ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16/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
17int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
18^
19__AVAILABILITY_INTERNAL_DEPRECATED 
20/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](/bitcoin-bitcoin/contributor/param/) query_parse the query portion of the URI
21          ^~~~~~~~~~~
22/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: note: did you mean 'uri'? [@param](/bitcoin-bitcoin/contributor/param/) query_parse the query portion of the URI
23          ^~~~~~~~~~~
24          uri
2569 warnings generated.

Note that a lot of these have already been fixed upstream.

build: suppress libevent warnings if supressing external warnings c6edcf1c71

doc: fixup -Wdocumentation issues 3b0078f958

build: enable -Wdocumentation if suppressing external warnings

Co-authored-by: Ben Woosley <ben.woosley@gmail.com>

a4e970adb6

fanquake added the label Docs on Apr 6, 2021

fanquake added the label Build system on Apr 6, 2021

MarcoFalke commented at 7:23 am on April 6, 2021: member

Concept ACK

laanwj commented at 7:48 am on April 6, 2021: member

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 a4e970adb6de8425025ae3f62fb89d9e27a8ab1f

practicalswift commented at 7:51 am on April 6, 2021: contributor

cr ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback

jonatack commented at 7:56 am on April 6, 2021: member

Approach ACK and nice fixes that this finds. This is handy indeed.

jonatack commented at 8:08 am on April 6, 2021: member

Light ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted

0In file included from test/addrman_tests.cpp:10:
1./netbase.h:237:11: warning: parameter 'sock' not found in the function declaration [-Wdocumentation]
2 * [@param](/bitcoin-bitcoin/contributor/param/) sock The SOCKS5 proxy socket.
3          ^~~~
4./netbase.h:237:11: note: did you mean 'socket'?
5 * [@param](/bitcoin-bitcoin/contributor/param/) sock The SOCKS5 proxy socket.
6          ^~~~
7          socket
81 warning generated.

fanquake merged this on Apr 7, 2021

fanquake closed this on Apr 7, 2021

sidhujag referenced this in commit ae43526fec on Apr 7, 2021

vasild commented at 9:04 am on April 12, 2021: member

Approach ACK

I think the new flags -Wdocumentation and -Werror=documentation could have been enabled unconditionally, regardless of whether --suppress-external-warnings is used. Similarly to other -Wfoo which cause warnings with 3rd party libs and we have enabled unconditionally.

fanquake commented at 9:06 am on April 12, 2021: member

I think the new flags -Wdocumentation and -Werror=documentation could have been enabled unconditionally, regardless of whether –suppress-external-warnings is used. Similarly to other -Wfoo which cause warnings with 3rd party libs and we have enabled unconditionally.

We can’t do this for the reasons outlined in the PR description. Anyone building with versions of upstream libraries where all -Wdocumentation issues hadn’t been fixed already, would be inundated with warnings at compile time.

fanquake deleted the branch on Sep 1, 2021

Fabcien referenced this in commit f83e0a6af7 on Jul 22, 2022

DrahtBot locked this on Sep 1, 2022

build: enable -Wdocumentation #21613