Skip to content
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.

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

Merge #16129 : refactor unused includes #4623

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Dec 15, 2021

Tried to Remove all unused includes, Some of the headers which are marked as removed in upstream changes but are still in use in are kept , hence number of File changes are appearing lesser than original changes

@github-actions
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp force-pushed the bp198 branch 2 times, most recently from 0b20ff2 to adfe89c Compare December 19, 2021 18:17
@vijaydasmp vijaydasmp changed the title Merge #16226: Move ismine to the wallet module Merge #16129 : refactor unused includes Dec 19, 2021
@vijaydasmp vijaydasmp force-pushed the bp198 branch 5 times, most recently from fa59710 to e43ffec Compare December 21, 2021 17:10
@vijaydasmp vijaydasmp marked this pull request as ready for review December 21, 2021 17:27
@vijaydasmp vijaydasmp marked this pull request as draft December 23, 2021 06:46
@vijaydasmp vijaydasmp force-pushed the bp198 branch 2 times, most recently from 602da86 to 3c9f9a3 Compare December 23, 2021 16:06
@github-actions
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp
Copy link
Author

Temp Test Failure Please Retrigger CI

@@ -65,7 +59,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
"llmq/signing -> net_processing -> llmq/signing"
"llmq/signing_shares -> net_processing -> llmq/signing_shares"
"logging -> util/system -> logging"
"logging -> util/system -> random -> logging"
Copy link
Author

@vijaydasmp vijaydasmp Dec 27, 2021

Choose a reason for hiding this comment

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

Removed "logging -> util/system -> random -> logging" as reported by linter that the dependency is no longer present

"logging -> util/system -> sync -> logging"
"logging -> util/system -> stacktraces -> logging"
"logging -> util/system -> util/getuniquepath -> random -> logging"
Copy link
Author

Choose a reason for hiding this comment

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

Added "logging -> util/system -> util/getuniquepath -> random -> logging" as reported by linter that the new dependency is introduced

67f4e9c Include core_io.h from core_read.cpp (practicalswift)
eca9767 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)

Pull request description:

  Make reasoning about dependencies easier by not including unused dependencies.

  Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.

  As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:

  ```
  $ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
        sed 's%^%src/%g' | xargs cat | wc -l
  51393
  ```

  Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)

ACKs for commit 67f4e9:

Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
@vijaydasmp vijaydasmp marked this pull request as ready for review December 27, 2021 11:22
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.

utACK

@github-actions
Copy link

This Pull Request may conflict if the Pull Requests below are merged first.

#4649
conflictable files: src/interfaces/node.cpp,src/qt/clientmodel.cpp

@github-actions
Copy link

This Pull Request may conflict if the Pull Requests below are merged first.

#4643
conflictable files: src/rpc/rawtransaction.cpp,src/wallet/rpcwallet.cpp,src/wallet/wallettool.cpp

@UdjinM6 UdjinM6 added this to the 18 milestone Jan 3, 2022
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.

I'm concerned if this actually only removes unused includes, since I previously programmatically removed unused includes... But, it compiles, so whatever

utACK for squash merge

@PastaPastaPasta PastaPastaPasta merged commit 3148450 into dashpay:develop Jan 3, 2022
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.

3 participants