build: Enable -Wdocumentation (clang) if available

practicalswift commented at 8:44 pm on August 8, 2018: contributor

Enable -Wdocumentation (clang) if available. -Wdocumentation emit warnings about incorrect use of documentation comments.
~~Fix broken Doxygen comments.~~

Example warnings:

0./policy/fees.h:25:6: warning: '@class' command should not be used in a comment attached to a non-class declaration [-Wdocumentation]
1qt/rpcconsole.cpp:147:18: warning: parameter 'result' not found in the function declaration [-Wdocumentation]
2./qt/bitcoingui.h:205:17: warning: parameter 'status' not found in the function declaration [-Wdocumentation]
3./wallet/wallet.h:992:38: warning: not a Doxygen trailing comment [-Wdocumentation]

in configure.ac:306 in dbdb908b6a outdated

302@@ -303,6 +303,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
303   AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
304   AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
305   AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
306+  AX_CHECK_COMPILE_FLAG([-Wdocumentation],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdocumentation"],,[[$CXXFLAG_WERROR]])

MarcoFalke commented at 8:52 pm on August 8, 2018:

meta-nit: Could add this in a line different from the last line to avoid merge conflicts with pulls that also append to this list of warnings.

practicalswift commented at 8:59 pm on August 8, 2018:

Good point! Fixed! :-)

MarcoFalke commented at 8:52 pm on August 8, 2018: member

Concept ACK. Didn’t know this existed.

practicalswift force-pushed on Aug 8, 2018

MarcoFalke added the label Docs on Aug 8, 2018

MarcoFalke commented at 9:33 pm on August 8, 2018: member

utACK 5705f5e5a9

MarcoFalke added this to the milestone 0.17.0 on Aug 8, 2018

MarcoFalke commented at 9:40 pm on August 8, 2018: member

Confirmed that the first commit does not change the objdump for me. Could go into 0.17 as bugfix, but I don’t care too strong.

fanquake commented at 1:20 am on August 9, 2018: member

NACK in it’s current form. When building on macOS, this generates, and fills the build output with hundreds of warnings from our dependencies (mostly libevent and openssl). Is there a way to ignore these?

 0In file included from httpserver.cpp:32:
 1In file included from ./support/events.h:12:
 2/usr/local/Cellar/libevent/2.1.8/include/event2/http.h:463: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.8/include/event2/http.h:464: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.8/include/event2/http.h:466: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.8/include/event2/http.h:936: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.8/include/event2/http.h:943:40: 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.8/include/event2/http.h:976: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.8/include/event2/http.h:984:66: 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.8/include/event2/http.h:999: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.8/include/event2/http.h:999: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.

 0/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1487:10: warning: parameter 'ev' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) ev an event struct
 1         ^~
 2/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1488:10: warning: parameter 'priority' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) priority the new priority to be assigned
 3         ^~~~~~~~
 4/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1558:11: warning: parameter 'base' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) base An event_base on which to scan the events.
 5          ^~~~
 6/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1559:11: warning: parameter 'output' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) output A stdio file to write on.
 7          ^~~~~~
 8/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1585:11: warning: parameter 'fd' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) fd The signal to active events on.
 9          ^~
10/usr/local/Cellar/libevent/2.1.8/include/event2/event.h:1585:11: note: did you mean 'sig'? [@param](/bitcoin-bitcoin/contributor/param/) fd The signal to active events on.
11          ^~
12          sig
13In file included from torcontrol.cpp:28:
14/usr/local/Cellar/libevent/2.1.8/include/event2/thread.h:187:11: warning: parameter 'base' not found in the function declaration [-Wdocumentation] [@param](/bitcoin-bitcoin/contributor/param/) base the event base for which to set the id function
15          ^~~~
1663 warnings generated.

 0   ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1/usr/local/Cellar/libevent/2.1.8/include/event2/http.h:984:66: note: add a deprecation attribute to the declaration to silence this warning
 2int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
 3                                                                 ^
 4                                                                   __deprecated
 5/usr/local/Cellar/libevent/2.1.8/include/event2/http.h:999: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
 6          ^~~~~~~~~~~
 7/usr/local/Cellar/libevent/2.1.8/include/event2/http.h:999:11: note: did you mean 'uri'? [@param](/bitcoin-bitcoin/contributor/param/) query_parse the query portion of the URI
 8          ^~~~~~~~~~~
 9          uri
1057 warnings generated.

 0In file included from ./qt/paymentrequestplus.h:16:
 1In file included from /usr/local/Cellar/openssl/1.0.2o_2/include/openssl/x509.h:87:
 2/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:295:14: warning: parameter 'flags' not found in the function declaration [-Wdocumentation]
 3 *   \param  flags flags value to set
 4             ^~~~~
 5/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:295:14: note: did you mean 'name'?
 6 *   \param  flags flags value to set
 7             ^~~~~
 8             name
 9/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:301:14: warning: parameter 'ecdsa_method' not found in the function declaration [-Wdocumentation]
10 *   \param  ecdsa_method  pointer to existing ECDSA_METHOD
11             ^~~~~~~~~~~~
12/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:302:14: warning: parameter 'name' not found in the function declaration [-Wdocumentation]
13 *   \param  name name to set
14             ^~~~
153 warnings generated.

MarcoFalke removed this from the milestone 0.17.0 on Aug 9, 2018

MarcoFalke commented at 1:56 am on August 9, 2018: member

@fanquake Thanks for testing on native mac. I presume they are not present when cross compiling?

Removed from the milestone for now.

MarcoFalke commented at 2:10 pm on August 10, 2018: member

I think developers don’t have to run with this warning enabled all the time. Could you just append it to the warnings that travis runs?

Empact commented at 9:11 pm on August 10, 2018: member

I am working on a way of fixing the dependency warning issue. The steps to that are:

Move tinyformat to a subtree or subdir (#13845 #13846)
Move dependency includes from -I to -isystem (wip here: https://github.com/Empact/bitcoin/tree/isystem)

Used -Wzero-as-null-pointer-constant to drive it out. Still needs work: https://github.com/Empact/bitcoin/tree/zero-as-null-pointer-constant

practicalswift commented at 10:14 am on August 13, 2018: contributor

@Empact Excellent! -isystem is the way to resolve this Would it be possible for you to open a PR with the -isystem change only? I could rebase this PR on top of that.

Empact commented at 5:57 pm on August 13, 2018: member

Waiting on one of the tinyformat changes to be merged, they’re tagged 0.18 so it will be a minute. You could rebase on the branch I list above, but it’s bound to change a bit before I open a PR so I would hold off.

luke-jr commented at 9:25 pm on August 27, 2018: member

Maybe we should have a --enable-developer-warnings option?

laanwj commented at 1:19 pm on August 29, 2018: member

I think fixing the documentation here is great, but I have to agree with @fanquake that as long as this covers the dependencies, this will generate way too much noise

practicalswift commented at 1:27 pm on August 29, 2018: contributor

@laanwj The plan is to make it not cover the dependencies using -isystem as @Empact suggested :-)

Empact commented at 6:30 am on August 30, 2018: member

@practicalswift how about updating this to just make the docs changes (as they can be helpful in the mean time) and we can do the other thing down the road (as that will take some time).

Enable -Wdocumentation (clang) if available 87df73b277

practicalswift force-pushed on Aug 30, 2018

practicalswift renamed this:
~~build: Enable -Wdocumentation (clang) if available. Fix broken Doxygen comments.~~
build: Enable -Wdocumentation (clang) if available
on Aug 30, 2018

practicalswift commented at 8:07 am on August 30, 2018: contributor

@Empact Good idea! Now keeping this PR for -Wdocumentation and opened a new PR #14103 for the broken Doxygen comments.

laanwj referenced this in commit be301a5777 on Aug 30, 2018

fanquake commented at 8:54 am on September 2, 2018: member

Going to close this for now, given that it’s reliant on other work, which might not be possible now that #13845 and #13846 aren’t being merged, see #13914 (comment).

If we find a way to fix all the warnings, then the flag can just be added at that time.

fanquake closed this on Sep 2, 2018

Empact commented at 3:07 pm on September 2, 2018: member

I’m going to put forward isystem at some point without the tinyformat changes. We could just disable the checks in the tinyformat file, given it is not subtree-checked.

practicalswift deleted the branch on Apr 10, 2021

Munkybooty referenced this in commit 197dfe05a7 on Jun 30, 2021

Munkybooty referenced this in commit 7f726d8172 on Jul 2, 2021

Munkybooty referenced this in commit 1cc7c3219d on Jul 2, 2021

gades referenced this in commit 8d3feeecb5 on May 24, 2022

DrahtBot locked this on Aug 18, 2022

build: Enable -Wdocumentation (clang) if available #13914