Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Jul 13, 2024

What was done?

Backport bitcoin#21613 to enable -Wdocumentation and related fixes for dash code

How Has This Been Tested?

Build with clang compiler and see no failures anymore with --enable-werror

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

fanquake and others added 3 commits July 13, 2024 00:41
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
@knst knst requested review from PastaPastaPasta and UdjinM6 July 14, 2024 12:40
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiles with no issues

utACK 252ffee

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 252ffee

@PastaPastaPasta PastaPastaPasta merged commit 04adca3 into dashpay:develop Jul 19, 2024
@knst knst deleted the bp-21613-Wdocumentation branch July 20, 2024 04:10
knst added a commit to knst/dash that referenced this pull request Jul 23, 2024
It happened due to 2 PRs merged one after each other without rebasing:
 - enabling -Wdocumentation in bitcoin#21631 (dashpay#6113)
 - renaming param while doxygen comment is forgotten in dashpay#6114
PastaPastaPasta added a commit that referenced this pull request Jul 23, 2024
…o wallet

bebf391 fix: build with -Wdocumentation - rename param pwallet to wallet (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  CI failures like that:
  https://gitlab.com/dashpay/dash/-/jobs/7400661581

  It happened due to 2 PRs merged one after each other without rebasing:
   - enabling -Wdocumentation in bitcoin#21631 (#6113)
   - renaming param while doxygen comment is forgotten in #6114

  ## What was done?
  It contains changes from #6133
  Thanks Vijay for spotting issue, this PR is DNM if 6133 will get merged faster.

  ## How Has This Been Tested?
  See CI

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK bebf391
  PastaPastaPasta:
    utACK bebf391

Tree-SHA512: 34265f09098c04593a9d52709e5b8aed8460248762655945e5c86a65adb9aa49ab6f0bdb21cd907d353fe6b10e1ff2e07d05528166bf8e2140eb2c9ea1acbd33
@UdjinM6 UdjinM6 added this to the 21.1 milestone Jul 23, 2024
@UdjinM6 UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants