-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
This pull request has conflicts, please rebase. |
0b20ff2
to
adfe89c
Compare
fa59710
to
e43ffec
Compare
602da86
to
3c9f9a3
Compare
This pull request has conflicts, please rebase. |
a272413
to
bddede9
Compare
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" |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
This Pull Request may conflict if the Pull Requests below are merged first. #4649 |
This Pull Request may conflict if the Pull Requests below are merged first. #4643 |
There was a problem hiding this 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
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