-
Notifications
You must be signed in to change notification settings - Fork 35.8k
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
Cleanup code by using forward declarations and other methods... #2767
Conversation
And how do you prevent very bad thing from happening when one of the many duplicated definitions gets desynchronized? |
Rebasing conflicts on the actual code changes made (one actual, moving some definitions from a header to the code file). Due diligence was taken on my part to ensure that the conflicts were resolved correctly, I would still recommend someone bite the bullet and look at all the changes that are not adding/removing/reordering header files. Rebasing additionally conflicted on include files being added/removed usually because most of the includes were re-ordered, these were easy to resolve. |
@gmaxwell From what I understand this does not duplicate anything. It does pre-declare classes, but that only consists of the name and nothing more. At first glance this seems like a lot of changes, but it's almost entirely restricted to the #include portion of files. The only substantial code move is moving WalletDB.h functions to its implementation file. There's problems with compiling the tests after this, though (see pulltester output).
|
|
||
|
||
#include <cstdio> |
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.
What is this needed for?
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.
The "printf"s on lines 406, 422, and 428.
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.
printf is #define'd in util.h to be OutputDebugStringF.
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.
Yeah, printf is our own macro.
I'm generally in favor of cleanups like this, and it indeed seems pure code movement + include changes. However:
@CodeShark Mind taking a look at this? Does it interfere with the dependencies cleanup? |
Getting rid of code dependencies is even better, but in the general case if it is possible do forward declarations instead of an #include in a header I'm in favor of that. It generates a flatter include hierarchy, let's not give the C++ compiler more work than need be. Re: uint64_t/int32_t I agree. If available, they should be used, as they take the burden of determining data type sizes from us, at least on most platforms. |
Addressed the specific problems references inline. Hopefully should pass build, don't know why it built on my machine to be honest. |
Delt with cstdio problems. Also normalized all [u]int64 types to [u]int64_t values from stdint.h, alongside replacing PRI64[xdu] with PRI[xdu]64 from inttypes.h . |
@brandondahler Simply replacing [u]int64 types with stdint will probably break something - we tried this like a year ago and had to revert it :( |
@luke-jr It was a non-negligable change, but it did compile and pass tests on my linux machine. Was the issue with compiling on other architectures? |
@brandondahler Reviewing logs, it looks like it didn't build on 64-bit Fedora. I don't believe we ever fully diagnosed the reason, but:
|
#include <mswsock.h> | ||
#include <winsock2.h> |
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.
winsock2.h must be included before mswsock.h or build fails.
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 also get a MinGW compiler warning that winsock2.h should be included before windows.h from this file in current master.
d:\mingw\mingw32\i686-w64-mingw32\include\winsock2.h:15: Warnung:#warning Please include winsock2.h before windows.h [-Wcpp] #warning Please include winsock2.h before windows.h ^
I think most other refactors that were in the pipeline are merged now. Care to rebase this? |
Rebased branch onto master. |
#include <QMenu> | ||
#include <QSortFilterProxyModel> |
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.
Is this needed, as we include it in addressbookpage.h?
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.
It is not included in addressbookpage.h--there is only forward declare (see below).
QT_BEGIN_NAMESPACE
...
class QSortFilterProxyModel;
...
QT_END_NAMESPACE
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.
My fault, I didn't look correctly, sorry :).
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.
No problems at all, thanks for reviewing the changes :)
It would be nice to collapse the commits, and merge in the [u]int64_t change first. Looks pretty good overall, though I do worry there is a subtle compiler detail being missed in the int64/int64_t type changes. Very much like seeing use of stdint.h and int64_t, rather than using our own type. |
I also like the intention of this pull, can you rebase and perhaps follow @jgarzik so we can differentiate the type-changes and the cleanup changes :). Edit: Also a merge-speedup is possible, if you create a Qt-only and a core-only pull, as @laanwj is able to merge Qt-pulls much faster than core changes. Edit 2: @laanwj Any objection, to start using quint64 or qint64 in Qt code? Perhaps that could also be a scope of a separate Qt pull. |
@@ -2,29 +2,36 @@ | |||
* W.J. van der Laan 2011-2012 | |||
*/ | |||
|
|||
|
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.
Nit: remove this new-line.
I really hope to see PullTester happy and some final ACKs for your great work here. |
Rebased and squashed small commits (made sure that the resuting rebased version is equivalent to the previously merged version). I plan on going over the source again to re-alphabetize etc. |
Pull-tester is fixed, so if it is complaining there is something wrong with your pull. See the test.log to debug. |
|
||
class CBlock; | ||
class CBlockHeader; |
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.
No need to forward declare this; CBlockHeader is defined in core, which is included.
Address @sipa comments. I think most of the main.h problems were caused by rebasing/merging so many times. |
@@ -17,6 +16,15 @@ | |||
#include "txdb.h" | |||
#include "txmempool.h" | |||
#include "ui_interface.h" | |||
#include "util.h" | |||
#include "wallet.h" |
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.
Since #3115, this shouldn't be needed anymore.
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.
Oh, this is my fault: it was supposed to remove setpwalletRegistered and its critical section, but apparently didn't. In any case, they're obsolete now, and that can remove the dependency on CWallet.
ACK, apart from my nits above. |
625188f Cleanup code using forward declarations. (Brandon Dahler)
Some suggested changes, which seem to work: sipa/bitcoin@0762f57 |
Use misc methods of avoiding unnecesary header includes. Replace int typedefs with int##_t from stdint.h. Replace PRI64[xdu] with PRI[xdu]64 from inttypes.h. Normalize QT_VERSION ifs where possible. Resolve some indirect dependencies as direct ones. Remove extern declarations from .cpp files.
Merged sipa/bitcoin@header-cleanup (sipa/bitcoin@0762f57) |
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/51ed9ec971614aebdbfbd9527aba365dd0afd437 for binaries and test log. |
ACK. Thanks a lot for the effort of writing this, and maintaining it. |
51ed9ec Cleanup code using forward declarations. (Brandon Dahler)
Rewrite `if (var = func())` in a less confusing way
... of avoiding unnecessary header includes. Additionally alphabetizes and groups includes and forward declarations.
Note: there are a large amount of changes that have been rebased onto the most recent master. I did my best to ensure that no code was functionally changed; however, a code review of all my changes would be advised.
Theory
By using forward declarations, compilation speed can be increased in both incremental and full builds.
Forward declarations suffice over having the full declaration in cases where the type that is being forward declared is only used as a reference or being pointed to, and the usage is not an lvalue.
By making these modifications, the total number of header files needed to compile any .cpp file is reduced. Fewer header files being included reduces the total amount of code that needs to be compiled for each .cpp file.
Quantitative Results
How much time does this actually account for?
For bitcoind:
![Bitcoind Results](https://camo.githubusercontent.com/80a3069e4eb169651b5eb51d5264549cea534e283325a87d73ad781590a1048b/687474703a2f2f692e696d6775722e636f6d2f4b4e44636742532e706e67)
This spreadsheet shows detailed results for each of the different builds (note that there are multiple sheets in the spreadsheet), along with the interesting stats and graph for each.
This data was generated automatically via the scripts here.
Method
Order
Most Generic
Example1 - No matching file, no file_in_other_dir, no ifdefs