Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Apr 6, 2021

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:

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.

@maflcko
Copy link
Member

maflcko commented Apr 6, 2021

Concept ACK

@laanwj
Copy link
Member

laanwj commented Apr 6, 2021

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

@practicalswift
Copy link
Contributor

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

@jonatack
Copy link
Member

jonatack commented Apr 6, 2021

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

Copy link
Member

@jonatack jonatack left a 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.

@fanquake fanquake merged commit 2b3e5bf into bitcoin:master Apr 7, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 7, 2021
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
@vasild
Copy link
Contributor

vasild commented Apr 12, 2021

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
Copy link
Member Author

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 fanquake deleted the enable_wdocumentation branch September 1, 2021 00:51
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 22, 2022
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants