Skip to content

util, refactor: Improve headers for bitcoin-wallet tool #23346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2021
Merged

util, refactor: Improve headers for bitcoin-wallet tool #23346

merged 1 commit into from
Dec 9, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Oct 24, 2021 •
edited
Loading

This PR:

  • removes unneeded #include <wallet/wallet.h> from <wallet/wallettool.h>
  • introduces class forward declaration in <wallet/wallettool.h>
  • added #include <config/bitcoin-config.h> to wallet/wallettool.cpp where the USE_BDB macro is used

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 24, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Oct 24, 2021
Merged
@hebasto
Copy link
Member Author

hebasto commented Oct 26, 2021

Rebased 5fc7c31 -> 49b906c (pr23346.01 -> pr23346.02) due to the conflict with #23006.

@DrahtBot DrahtBot mentioned this pull request Oct 26, 2021
Draft
stratospher
stratospher reviewed Oct 27, 2021
View reviewed changes
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

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

ACK 49b906c. Verified that:

  • #include <wallet/wallet.h> isn’t needed in src/wallet/wallettool.h
  • All the newly added #include in the files are necessary.

I’m curious to know why WalletShowInfo() was moved out of src/wallet/wallettool.h though.

@hebasto
Copy link
Member Author

hebasto commented Oct 27, 2021

I’m curious to know why WalletShowInfo() was moved out of src/wallet/wallettool.h though.

It is used in the only compilation unit:

bitcoin/src/wallet/wallettool.cpp

Line 100 in 49b906c

static void WalletShowInfo(CWallet* wallet_instance)

luke-jr
luke-jr reviewed Oct 29, 2021
View reviewed changes
src/bitcoin-wallet.cpp Outdated
Comment on lines 5 to 8
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

Copy link
Member

Choose a reason for hiding this comment

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

This should be left in IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@hebasto
Copy link
Member Author

hebasto commented Oct 30, 2021

Updated 49b906c -> 3431839 (pr23346.02 -> pr23346.03, diff):

This was referenced Nov 2, 2021
Draft
Draft
@Sjors
Copy link
Member

Sjors commented Dec 9, 2021

Lgtm, @achow101?

@maflcko maflcko merged commit 7ce8d74 into bitcoin:master Dec 9, 2021
maflcko
maflcko reviewed Dec 9, 2021
View reviewed changes
src/wallet/wallettool.cpp
#include <config/bitcoin-config.h>
#endif

#include <wallet/wallettool.h>
Copy link
Member

Choose a reason for hiding this comment

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

doesn't matter here, but the self-include is always the very first one on its own section for us, I think

@hebasto hebasto deleted the 211024-bw-headers branch December 9, 2021 13:17
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2021
Merge bitcoin#23346: util, refactor: Improve headers for bitcoin-wall…
d07304d 
…et tool

3431839 util, refactor: Improve headers for bitcoin-wallet tool (Hennadii Stepanov)

Pull request description:

  This PR:
  - removes unneeded `#include <wallet/wallet.h>` from `<wallet/wallettool.h>`
  - introduces class forward declaration in `<wallet/wallettool.h>`
  - added `#include <config/bitcoin-config.h>` to `wallet/wallettool.cpp` where the `USE_BDB` macro is used

Top commit has no ACKs.

Tree-SHA512: a0de560d821f8b570ae806a1165b9b382c9e0b339687d932052fa4c38ab2ba493e7e050f19adc02ad7db40c42cf88ac1d37209f9071494a0ab268ed33ff22b9f
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
Merge bitcoin/bitcoin#23346: util, refactor: Improve headers for bitc…
8d6a2c5 
…oin-wallet tool

d99b3a7 util, refactor: Improve headers for bitcoin-wallet tool (Hennadii Stepanov)

Pull request description:

  This PR:
  - removes unneeded `#include <wallet/wallet.h>` from `<wallet/wallettool.h>`
  - introduces class forward declaration in `<wallet/wallettool.h>`
  - added `#include <config/bitcoin-config.h>` to `wallet/wallettool.cpp` where the `USE_BDB` macro is used

Top commit has no ACKs.

Tree-SHA512: a0de560d821f8b570ae806a1165b9b382c9e0b339687d932052fa4c38ab2ba493e7e050f19adc02ad7db40c42cf88ac1d37209f9071494a0ab268ed33ff22b9f
@bitcoin bitcoin locked and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@luke-jr luke-jr luke-jr left review comments

@maflcko maflcko maflcko left review comments

@stratospher stratospher stratospher left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Utils/log/libs Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @DrahtBot @Sjors @luke-jr @maflcko @stratospher