refactor: move GetServicesNames from rpc/util.{h,cpp} to rpc/net.cpp #28136

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:2023-07-move-GetServicesNames-to-its-call-unit changing 6 files +16 −16

jonatack commented at 1:34 AM on July 24, 2023: member

Move GetServicesNames() from rpc/util to rpc/net.cpp, as it is only called from that compilation unit and there is no reason for other ones to need it.

Remove the protocol.h include in rpc/util.h, as it was only needed for GetServicesNames(), drop an unneeded forward declaration (the other IWYU suggestions would require more extensive changes in other files), and add 3 already-missing include headers in other translation units that are needed to compile without protocol.h in rpc/util.h, as protocol.h includes netaddress.h, which in turn includes util/strencodings.h.
DrahtBot commented at 1:34 AM on July 24, 2023: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK kevkevinpal, ns-xvrn, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22341 (rpc: add getxpub by Sjors)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Refactoring on Jul 24, 2023
jonatack force-pushed on Jul 24, 2023
jonatack marked this as ready for review on Jul 24, 2023
DrahtBot added the label CI failed on Sep 15, 2023
jonatack commented at 5:44 PM on September 19, 2023: member

Re the new CI failure, seems an update on master now causes a build error here. Updating.

Type	Reviewers
ACK	kevkevinpal, ns-xvrn, achow101

refactor: move GetServicesNames from rpc/util.{h,cpp} to rpc/net.cpp

as it is only called from that compilation unit.

This avoids needlessly compiling GetServicesNames() in the 35 other files that
include rpc/util.h.

1dd62c5295

jonatack force-pushed on Sep 19, 2023

refactor: drop protocol.h include header in rpc/util.h

as it was only needed for GetServicesNames(). This potentially avoids needlessly
compiling the 500 lines of protocol.h in the 35 files other than rpc/net.cpp
that include rpc/util.h.

Drop an unneeded CPubKey forward declaration. The other IWYU suggestions would
require more extensive changes in other files.

Add 3 already-missing include headers in other translation units that are needed
to compile without protocol.h in rpc/util.h, as it includes netaddress.h, which
in turn includes util/strencodings.h.

bbb68ffdbd

jonatack force-pushed on Sep 19, 2023
DrahtBot removed the label CI failed on Sep 19, 2023
jonatack commented at 11:33 PM on September 19, 2023: member

Rebased and updated. The Win64 CI error is unrelated and documented in issue #28491.
kevkevinpal commented at 3:37 AM on September 21, 2023: contributor

lgtm ACK bbb68ff
nsvrn commented at 8:01 PM on October 13, 2023: contributor

All unit and functional tests passed locally on my Linux system. Refactoring makes sense.

ACK bbb68ff
achow101 commented at 7:13 PM on November 7, 2023: member

ACK bbb68ffdbdafb6717dcadac074f6098750b8aa77
achow101 merged this on Nov 7, 2023
achow101 closed this on Nov 7, 2023
jonatack deleted the branch on Nov 7, 2023
bitcoin locked this on Nov 6, 2024

Contributors

Labels

refactor: move GetServicesNames from rpc/util.{h,cpp} to rpc/net.cpp #28136

Code Coverage

Reviews

Conflicts