refactoring: Remove unused includes #16659

practicalswift · 2019-08-19T16:28:33Z

As requested by MarcoFalke in #16273 (comment):

This PR removes unused includes.

Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.

I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup.

Rationale:

Avoids unnecessary re-compiles in case of header changes.
Makes reasoning about code dependencies easier.
Reduces compile-time memory usage.
Reduces compilation time.
Warm fuzzy feeling of being lean :-)

GChuf · 2019-08-19T17:03:04Z

I'm all for lean and mean stuff.
Concept ACK

jb55 · 2019-08-19T17:59:31Z

Concept ACK

DrahtBot · 2019-08-19T18:28:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#15934 (Merge settings one place instead of five places by ryanofsky)
#14053 (Add address-based index (attempt 4?) by marcinja)
#13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)

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.

practicalswift · 2019-10-16T05:41:52Z

Rebased again :)

I think this one should be ready for final code review!

ryanofsky

Code review ACK 084e17c. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway.

I would encourage @practicalswift to be more transparent about tools and methodologies used, since it's unclear how this PR was generated, how the changes should be maintained, and if there were tools used that might deserve credit.

practicalswift · 2019-10-16T15:29:08Z

@ryanofsky The only tools used are nm and cxxfilt (python module). The rest is a very much Bitcoin Core specific Python script I've written myself :)

$ wc -l find_redundant_includes.??
 2207 find_redundant_includes.py
    3 find_redundant_includes.sh
 2210 total
$ grep -v '"' find_redundant_includes.py | wc -l
371

2207 LOC might sound crazy but a lot of it is taken up by a.) a mapping between object file names and headers file names and b.) a mapping between standard library headers and standard library functions/symbols. The real line count should be somewhere around 400 LOC.

It is not pretty but it works :)

ryanofsky · 2019-10-16T15:55:50Z

Thanks. I'd be curious to see the script if you want to post it somewhere like a gist.

084e17c Remove unused includes (practicalswift) Pull request description: As requested by MarcoFalke in #16273 (comment): This PR removes unused includes. Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier. I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup. Rationale: * Avoids unnecessary re-compiles in case of header changes. * Makes reasoning about code dependencies easier. * Reduces compile-time memory usage. * Reduces compilation time. * Warm fuzzy feeling of being lean :-) ACKs for top commit: ryanofsky: Code review ACK 084e17c. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway. Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d

maflcko

ACK

maflcko · 2019-10-16T21:44:19Z

src/node/transaction.cpp

@@ -6,7 +6,6 @@
 #include <consensus/validation.h>
 #include <net.h>
 #include <net_processing.h>
-#include <txmempool.h>


This is actually used

Oh, I'll investigate shortly! Did you find any other examples? Was it found by manual review?

practicalswift · 2019-10-16T23:07:55Z

Thanks for merging!

See below for a script run after this merge.

I believe the remaining cases to be of the "non-trivial" type where the header directly included is not needed in itself but one of more of the transitively included headers are.

Consider the src/bench/rpc_blockchain.cpp example: we are currently including validation.h which transitively gives us PROTOCOL_VERSION, CBlock, CBlockIndex and uint256. But we don't need anything validation.h provides in itself.

Instead of including validation.h we should include version.h for PROTOCOL_VERSION, primitives/block.h for CBlock, chain.h for CBlockIndex and uint256.h for uint256.

Like this:

diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
index 2fc6f116a..00cec5ecb 100644
--- a/src/bench/rpc_blockchain.cpp
+++ b/src/bench/rpc_blockchain.cpp
@@ -5,9 +5,12 @@
 #include <bench/bench.h>
 #include <bench/data.h>
 
-#include <validation.h>
+#include <chain.h>
+#include <primitives/block.h>
 #include <streams.h>
 #include <rpc/blockchain.h>
+#include <uint256.h>
+#include <version.h>
 
 #include <univalue.h>

$ ./find_redundant_includes.sh
src/bench/rpc_blockchain.cpp -- unused include: validation.h
src/bench/verify_script.cpp -- unused include: script/standard.h
src/coins.h -- unused include: core_memusage.h
src/core_read.cpp -- unused include: script/sign.h
src/crypto/hkdf_sha256_32.h -- unused include: crypto/hmac_sha256.h
src/index/txindex.h -- unused include: txdb.h
src/net.h -- unused include: addrdb.h
src/net.h -- unused include: limitedmap.h
src/net.h -- unused include: policy/feerate.h
src/noui.cpp -- unused include: util/system.h
src/psbt.h -- unused include: node/transaction.h
src/psbt.h -- unused include: optional.h
src/psbt.h -- unused include: policy/feerate.h
src/qt/bantablemodel.h -- unused include: net.h
src/qt/receivecoinsdialog.cpp -- unused include: wallet/wallet.h
src/qt/sendcoinsdialog.cpp -- unused include: txmempool.h
src/qt/sendcoinsdialog.cpp -- unused include: wallet/fees.h
src/qt/signverifymessagedialog.cpp -- unused include: wallet/wallet.h
src/qt/walletmodel.h -- unused include: script/standard.h
src/qt/winshutdownmonitor.cpp -- unused include: util/system.h
src/rpc/misc.cpp -- unused include: rpc/blockchain.h
src/rpc/util.h -- unused include: node/transaction.h
src/rpc/util.h -- unused include: script/sign.h
src/script/sign.h -- unused include: streams.h
src/support/events.h -- unused include: ios
src/sync.h -- unused include: thread
src/test/fuzz/FuzzedDataProvider.h -- unused include: utility
src/test/fuzz/eval_script.cpp -- unused include: limits
src/test/key_tests.cpp -- unused include: util/system.h
src/txmempool.h -- unused include: random.h
src/uint256.h -- unused include: assert.h
src/util/system.h -- unused include: util/memory.h
src/util/system.h -- unused include: util/time.h
src/wallet/coinselection.h -- unused include: random.h
src/wallet/crypter.h -- unused include: script/signingprovider.h
src/wallet/test/wallet_tests.cpp -- unused include: rpc/server.h
src/wallet/wallet.cpp -- unused include: util/validation.h
src/wallet/walletdb.h -- unused include: script/sign.h
src/zmq/zmqabstractnotifier.h -- unused include: zmq/zmqconfig.h
src/zmq/zmqconfig.h -- unused include: primitives/transaction.h
src/zmq/zmqconfig.h -- unused include: stdarg.h

Anyone who wants to address the above cases too? Then we should be as lean as it gets when it comes to redundant includes :)

If you do: let me know if you find any false positives/false negatives given the list above.

One caveat is that I'm currently skipping analysis for core_io.h due to the weird naming mismatch between core_io.h ⇄ core_write.cpp/core_read.cpp :)

laanwj · 2019-10-17T08:58:34Z

One caveat is that I'm currently skipping analysis for core_io.h

It suppose that is a trivial header anyway, memory usage wise. It's a few simple functions. It doesn't pull in any boost nor non-run-of-the-mill C++ standard library headers.

084e17c Remove unused includes (practicalswift) Pull request description: As requested by MarcoFalke in bitcoin#16273 (comment): This PR removes unused includes. Please note that in contrast to bitcoin#16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier. I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup. Rationale: * Avoids unnecessary re-compiles in case of header changes. * Makes reasoning about code dependencies easier. * Reduces compile-time memory usage. * Reduces compilation time. * Warm fuzzy feeling of being lean :-) ACKs for top commit: ryanofsky: Code review ACK 084e17c. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway. Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d

Summary: 91509ffe247b0eacbf84214c7c9c3f8a0012f2eb bench: Benchmark blockToJSON (Kirill Fomichev) Pull request description: Related: - "getblock performance issue on verbosity" bitcoin/bitcoin#15925 - "refactor: Avoid UniValue copy constructor" #15974 ACKs for top commit: laanwj: ACK 91509ffe247b0eacbf84214c7c9c3f8a0012f2eb Tree-SHA512: e70b12cb31921c7527bde334f52f39776da698b6bbdb196079a8b68478c67585a5bd7bed7403f65166bd604f7ed60778c53dc064d743bb8368318a1283d1073e Backport of Core [[bitcoin/bitcoin#16267 | PR16267]] Also cleans up the includes per Core [[bitcoin/bitcoin#16659 | PR16659]] Depends on D5980 Test Plan: ``` ninja check ninja src/bench/bitcoin-bench ./src/bench/bitcoin-bench -filter=BlockToJsonVerbose ``` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5981

Summary: 084e17cebd424b8e8ced674bc810eef4e6ee5d3b Remove unused includes (practicalswift) Pull request description: As requested by MarcoFalke in bitcoin/bitcoin#16273 (comment): This PR removes unused includes. Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier. I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup. Rationale: * Avoids unnecessary re-compiles in case of header changes. * Makes reasoning about code dependencies easier. * Reduces compile-time memory usage. * Reduces compilation time. * Warm fuzzy feeling of being lean :-) ACKs for top commit: ryanofsky: Code review ACK 084e17cebd424b8e8ced674bc810eef4e6ee5d3b. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway. --- Backport of Core [[bitcoin/bitcoin#16659 | PR16659]] Test Plan: ninja check-all ninja Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6586

0eabb2a build: Remove unused header from the build system (Hennadii Stepanov) Pull request description: The only `#include <miniupnpc/miniwget.h>` was removed in #16659. ACKs for top commit: practicalswift: cr ACK 0eabb2a fanquake: ACK 0eabb2a Tree-SHA512: 630da03875c851e80286561eae0f966c89624cbb17b90f70e2bec9a69146e79d088fc176e07a4906915770ac1cdb11341a7a431ea7cf6a59d2816e927486f335

0eabb2a build: Remove unused header from the build system (Hennadii Stepanov) Pull request description: The only `#include <miniupnpc/miniwget.h>` was removed in bitcoin#16659. ACKs for top commit: practicalswift: cr ACK 0eabb2a fanquake: ACK 0eabb2a Tree-SHA512: 630da03875c851e80286561eae0f966c89624cbb17b90f70e2bec9a69146e79d088fc176e07a4906915770ac1cdb11341a7a431ea7cf6a59d2816e927486f335

084e17c Remove unused includes (practicalswift) Pull request description: As requested by MarcoFalke in bitcoin#16273 (comment): This PR removes unused includes. Please note that in contrast to bitcoin#16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier. I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup. Rationale: * Avoids unnecessary re-compiles in case of header changes. * Makes reasoning about code dependencies easier. * Reduces compile-time memory usage. * Reduces compilation time. * Warm fuzzy feeling of being lean :-) ACKs for top commit: ryanofsky: Code review ACK 084e17c. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway. Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d

Zcash: Also includes unused include cleanup from bitcoin/bitcoin#16659. (cherry picked from commit bitcoin/bitcoin@8508473)

maflcko added this to the Future milestone Aug 19, 2019

DrahtBot added Consensus GUI Mempool Mining P2P Refactoring RPC/REST/ZMQ Tests TX fees and policy Utils/log/libs UTXO Db and Indexes Validation Wallet labels Aug 19, 2019

fanquake added Needs Conceptual Review and removed Consensus GUI Mempool Mining P2P RPC/REST/ZMQ TX fees and policy Tests UTXO Db and Indexes Utils/log/libs Validation Wallet labels Aug 19, 2019

practicalswift force-pushed the unused-includes branch from bf0da68 to 2d54625 Compare October 14, 2019 21:10

DrahtBot added the Needs rebase label Oct 15, 2019

practicalswift force-pushed the unused-includes branch from 2d54625 to 32cc30f Compare October 15, 2019 22:54

Remove unused includes

084e17c

practicalswift force-pushed the unused-includes branch from 32cc30f to 084e17c Compare October 15, 2019 22:56

DrahtBot removed the Needs rebase label Oct 15, 2019

ryanofsky approved these changes Oct 16, 2019

View reviewed changes

maflcko merged commit 084e17c into bitcoin:master Oct 16, 2019

maflcko reviewed Oct 16, 2019

View reviewed changes

practicalswift mentioned this pull request Oct 29, 2019

refactor: Remove addrdb.h dependency from node.h #17297

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

hebasto mentioned this pull request Mar 22, 2021

build: Remove unused header from the build system #21505

Merged

practicalswift deleted the unused-includes branch April 10, 2021 19:38

str4d pushed a commit to zcash/zcash that referenced this pull request Jul 16, 2022

Avoid non-trivial global constants in SHA-NI code

c4e9e1c

Zcash: Also includes unused include cleanup from bitcoin/bitcoin#16659. (cherry picked from commit bitcoin/bitcoin@8508473)

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactoring: Remove unused includes #16659

refactoring: Remove unused includes #16659

Uh oh!

practicalswift commented Aug 19, 2019

Uh oh!

GChuf commented Aug 19, 2019

Uh oh!

jb55 commented Aug 19, 2019

Uh oh!

DrahtBot commented Aug 19, 2019 •

edited

Loading

Uh oh!

practicalswift commented Oct 16, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

practicalswift commented Oct 16, 2019 •

edited

Loading

Uh oh!

ryanofsky commented Oct 16, 2019

Uh oh!

maflcko left a comment

Uh oh!

maflcko Oct 16, 2019

Uh oh!

practicalswift Oct 16, 2019 •

edited

Loading

Uh oh!

practicalswift commented Oct 16, 2019

Uh oh!

laanwj commented Oct 17, 2019

Uh oh!

Uh oh!

refactoring: Remove unused includes #16659

refactoring: Remove unused includes #16659

Uh oh!

Conversation

practicalswift commented Aug 19, 2019

Uh oh!

GChuf commented Aug 19, 2019

Uh oh!

jb55 commented Aug 19, 2019

Uh oh!

DrahtBot commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Oct 16, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 16, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 16, 2019

Uh oh!

laanwj commented Oct 17, 2019

Uh oh!

Uh oh!

DrahtBot commented Aug 19, 2019 •

edited

Loading

practicalswift commented Oct 16, 2019 •

edited

Loading

practicalswift Oct 16, 2019 •

edited

Loading