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

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

Example warnings:

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

Concept ACK. Didn't know this existed.

utACK 5705f5e5a9

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.

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?

In file included from httpserver.cpp:32:
In file included from ./support/events.h:12:
/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
          ^~~
/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.
          ^~~~~~~
/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.
          ^~~~
/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
  ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/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
char *evhttp_decode_uri(const char *uri);
                                       ^
                                         __AVAILABILITY_INTERNAL_DEPRECATED
/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
   ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/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
int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
                                                                 ^
                                                                   __AVAILABILITY_INTERNAL_DEPRECATED
/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
          ^~~~~~~~~~~
/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
          ^~~~~~~~~~~
          uri
69 warnings generated.

/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
         ^~
/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
         ^~~~~~~~
/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.
          ^~~~
/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.
          ^~~~~~
/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.
          ^~
/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.
          ^~
          sig
In file included from torcontrol.cpp:28:
/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
          ^~~~
63 warnings generated.

   ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/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
int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
                                                                 ^
                                                                   __deprecated
/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
          ^~~~~~~~~~~
/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
          ^~~~~~~~~~~
          uri
57 warnings generated.

In file included from ./qt/paymentrequestplus.h:16:
In file included from /usr/local/Cellar/openssl/1.0.2o_2/include/openssl/x509.h:87:
/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:295:14: warning: parameter 'flags' not found in the function declaration [-Wdocumentation]
 *   \param  flags flags value to set
             ^~~~~
/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:295:14: note: did you mean 'name'?
 *   \param  flags flags value to set
             ^~~~~
             name
/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]
 *   \param  ecdsa_method  pointer to existing ECDSA_METHOD
             ^~~~~~~~~~~~
/usr/local/Cellar/openssl/1.0.2o_2/include/openssl/ecdsa.h:302:14: warning: parameter 'name' not found in the function declaration [-Wdocumentation]
 *   \param  name name to set
             ^~~~
3 warnings generated.

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

Removed from the milestone for now.

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?

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

1. Move tinyformat to a subtree or subdir (#13845 #13846)
2. 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

@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.

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.

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

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

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

@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).

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

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.

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.