Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 19, 2022

Unused includes are confusing, but also cause unrelated compile errors when the unused includes were to be removed.

Fix that by adding the missing includes where they are needed and then remove them where they are not needed. This is also checked by iwyu.

MacroFake added 2 commits July 19, 2022 14:12
This is needed for the next commit
They are needed, otherwise the next commit will not compile
@hebasto
Copy link
Member

hebasto commented Jul 19, 2022

Concept ACK.

@fanquake
Copy link
Member

ACK if green tidy. Could also include

rpc/fees.cpp should remove these lines:
- #include <policy/policy.h>  // lines 9-9
- #include <util/system.h>  // lines 19-19

@maflcko maflcko force-pushed the 2207-include-dbwrapper- branch from fa622e0 to faf98ae Compare July 19, 2022 12:34
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK faf98ae, I have reviewed the code and it looks OK, I agree it can be merged.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25623 ([kernel 3e/n] Decouple CDBWrapper and CBlockTreeDB from ArgsManager by dongcarl)
  • #25527 ([kernel 3c/n] Decouple validation cache initialization from ArgsManager by dongcarl)
  • #25172 (refactor: use std:: prefix for std lib funcs by fanquake)
  • #24833 (refactor: consensus/tx_verify.{h,cpp} tidy-ups by jonatack)
  • #24232 (assumeutxo: add init and completion logic by jamesob)

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.

@maflcko maflcko changed the title Remove unused includes from dbwrapper.h refactor: Remove unused includes from dbwrapper.h Jul 19, 2022
@Sjors
Copy link
Member

Sjors commented Jul 19, 2022

It indeed does look a bit confusing. But I can still build from scratch on macOS 12.4

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Code Review ACK faf98ae

CI is green, compiled on ubuntu 22.04 and macOS 12.4 without issue, also ran tests.

@fanquake fanquake merged commit 5560682 into bitcoin:master Jul 19, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022
faf98ae Remove unused includes in rpc/fees.cpp (MacroFake)
1111dde Remove unused includes from dbwrapper.h (MacroFake)
fa77fdd Add missing includes (MacroFake)
fa869ce Add missing includes to node/chainstate (MacroFake)

Pull request description:

  Unused includes are confusing, but also cause unrelated compile errors when the unused includes were to be removed.

  Fix that by adding the missing includes where they are needed and then remove them where they are not needed. This is also checked by iwyu.

ACKs for top commit:
  hebasto:
    ACK faf98ae, I have reviewed the code and it looks OK, I agree it can be merged.
  jarolrod:
    Code Review ACK bitcoin@faf98ae

Tree-SHA512: 75f3c6e6f6ecf8a98233e1a1463c75ca4e0eb3ec341150d274141072fe95413a3c2ec6386d1c527899cc63d43f63f5eb5991509847412773362808ddfb1bb435
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022
@maflcko maflcko deleted the 2207-include-dbwrapper-🔙 branch July 20, 2022 05:53
@bitcoin bitcoin locked and limited conversation to collaborators Jul 20, 2023
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