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

practicalswift · 2018-08-08T20:44:25Z

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]

maflcko · 2018-08-08T20:52:03Z

configure.ac

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


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.

Good point! Fixed! :-)

maflcko · 2018-08-08T20:52:47Z

Concept ACK. Didn't know this existed.

maflcko · 2018-08-08T21:33:16Z

utACK 5705f5e5a9

maflcko · 2018-08-08T21:40:52Z

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 · 2018-08-09T01:20:51Z

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 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 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 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  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 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 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 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 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 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 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 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 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 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 base the event base for which to set the id function
          ^~~~
63 warnings generated.

   @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);
                                                                 ^
                                                                   __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 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 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.

maflcko · 2018-08-09T01:56:59Z

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

Removed from the milestone for now.

maflcko · 2018-08-10T14:10:56Z

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 · 2018-08-10T21:11:33Z

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

Move tinyformat to a subtree or subdir (Include tinyformat as a subtree #13845 Move src/tinyformat.h to src/tinyformat/tinyformat.h #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 · 2018-08-13T10:14:04Z

@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 · 2018-08-13T17:57:07Z

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 · 2018-08-27T21:25:24Z

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

laanwj · 2018-08-29T13:19:12Z

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 · 2018-08-29T13:27:24Z

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

Empact · 2018-08-30T06:30:35Z

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

practicalswift · 2018-08-30T08:07:24Z

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

0e534d4 Fix incorrect Doxygen comments (practicalswift) Pull request description: Fix broken Doxygen comments. This commit was taken from #13914 which now only covers `-Wdocumentation`. Tree-SHA512: dddbca16bb792b8193e5f417151b5eace9acc942a321f1bc095b906e98889e3bd93509fe112ab6a24ee1f6a3a918db905bda7acefd53774fe3e6ebe669fb51ac

fanquake · 2018-09-02T08:54:26Z

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.

Empact · 2018-09-02T15:07:42Z

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.

0e534d4 Fix incorrect Doxygen comments (practicalswift) Pull request description: Fix broken Doxygen comments. This commit was taken from bitcoin#13914 which now only covers `-Wdocumentation`. Tree-SHA512: dddbca16bb792b8193e5f417151b5eace9acc942a321f1bc095b906e98889e3bd93509fe112ab6a24ee1f6a3a918db905bda7acefd53774fe3e6ebe669fb51ac # Conflicts: # src/policy/fees.h # src/wallet/wallet.h

maflcko reviewed Aug 8, 2018

View reviewed changes

practicalswift force-pushed the Wdocumentation branch from dbdb908 to 5705f5e Compare August 8, 2018 20:58

maflcko added the Docs label Aug 8, 2018

maflcko added this to the 0.17.0 milestone Aug 8, 2018

maflcko removed this from the 0.17.0 milestone Aug 9, 2018

practicalswift mentioned this pull request Aug 30, 2018

docs: Fix broken Doxygen comments #14103

Merged

Enable -Wdocumentation (clang) if available

87df73b

practicalswift force-pushed the Wdocumentation branch from 5705f5e to 87df73b Compare August 30, 2018 08:06

practicalswift changed the title ~~build: Enable -Wdocumentation (clang) if available. Fix broken Doxygen comments.~~ build: Enable -Wdocumentation (clang) if available Aug 30, 2018

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

fanquake closed this Sep 2, 2018

Empact mentioned this pull request Dec 11, 2018

Build: enable -Wdocumentation via isystem #14920

Closed

practicalswift deleted the Wdocumentation branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

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

Uh oh!

Conversation

practicalswift commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 8, 2018

Uh oh!

maflcko commented Aug 8, 2018

Uh oh!

maflcko commented Aug 8, 2018

Uh oh!

fanquake commented Aug 9, 2018

Uh oh!

maflcko commented Aug 9, 2018

Uh oh!

maflcko commented Aug 10, 2018

Uh oh!

Empact commented Aug 10, 2018

Uh oh!

practicalswift commented Aug 13, 2018

Uh oh!

Empact commented Aug 13, 2018

Uh oh!

luke-jr commented Aug 27, 2018

Uh oh!

laanwj commented Aug 29, 2018

Uh oh!

practicalswift commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 30, 2018

Uh oh!

fanquake commented Sep 2, 2018

Uh oh!

Empact commented Sep 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Aug 8, 2018 •

edited

Loading

practicalswift commented Aug 29, 2018 •

edited

Loading

Empact commented Aug 30, 2018 •

edited

Loading

Empact commented Sep 2, 2018 •

edited

Loading