MOVEONLY-ish: Do not include main.h from any other header #5681

pull jtimon wants to merge 3 commits into bitcoin:master from jtimon:main_includes changing 17 files +221 −160
  1. jtimon commented at 7:59 pm on January 19, 2015: contributor
    Moving some code, including main.h from qt/peertablemodel.h and wallet.h can be avoided. Also, main.h doesn’t need to include many things that can be included in main.cpp. After that, using the compiler to make sure only files that depend on main.h include it, many missing includes show up. Many files should require less memory for being compiled after this.
  2. jtimon force-pushed on Jan 19, 2015
  3. theuni commented at 2:35 am on January 20, 2015: member

    Very nice, I’ve been meaning to take a look at this as well. Great to see it was relatively non-invasive. I’ve verified that fd10da8cae79a15d2ac23a17120cb2d62e8a28e2 and b46dd0f3f53ec20035660aaca07484f898df1fc5 are read-only. From a quick eye-ball (and since Travis is happy), the includes changes look good too.

    utAck.

  4. laanwj added the label Improvement on Jan 20, 2015
  5. laanwj commented at 10:36 am on January 20, 2015: member

    ACK. Looks like a definite improvement to me, and low-risk due to (mostly) move only.

    It becomes kind of apparent from this how much is exported in main.h that

    • a) initialization/shutdown purposes (InitBlockIndex/LoadBlockIndex/UnloadBlockIndex/ThreadScriptCheck at least).
    • b) only used lnternally (WriteBlockToDisk, ProcessMessages, GetMinRelayFee at least), but see c - possibly this means they need tests
    • c) exported only for testing and could be static/internal otherwise (CScriptCheck)
    • d) pure utility (AreInputsStandard, GetLegacySigOpCount, IsStandardTx, GetP2SHSigOpCount)

    (a) could be reduced by factoring the initialization and shutdown sequences from init.cpp and moving them with what is initiialized (b) could be removed from the header, or tests added (c) is always thorny, you need to expose them but only to tests, could have a special “test interface” (d) could be moved to a standard.cpp/standard.h @sipa may want to weigh in here too as he has specific ideas about refactoring main.

  6. jtimon force-pushed on Jan 20, 2015
  7. jtimon commented at 8:33 pm on January 20, 2015: contributor

    For b and c, maybe a half-solution that just comes to mind, is just leaving the definitions in main.cpp but expose them with an independent header like main_test.h or something. Just a random thought.

    I’m basically focusing on consensus and policy checks. Trying to take advantage of potential similarities between their dependencies. Right now it’s kind of a mess that doesn’t even pass the tests, but if you’re curious, here’s my latest branch on that: https://github.com/jtimon/bitcoin/commits/consensus_policy I guess it mostly falls into the “d” category, with a little bit of “a” as you move constants and globals out. This (and therefore #5669 and the longest branch) needed rebase. I’m also trying to isolate a good small base for policy to use as an example and confirm what I think @luke-jr and I (and hopefully everyone) can agree on.

  8. Diapolo commented at 9:56 pm on January 20, 2015: none
    Pretty cool, that you are following the ordering style still ^^. Does this also speedup compilation times, did you benchmark it?
  9. jtimon commented at 11:05 am on January 21, 2015: contributor
    @Diapolo thanks for double-checking the include alphabetical order. I believe it does speedup compilation times, but I haven’t benchmarked it. Some benchmarks would be great if you want to do them.
  10. jtimon force-pushed on Jan 23, 2015
  11. jtimon commented at 1:04 pm on January 23, 2015: contributor
    I left the final commit cleaning up the includes for later. It is the hardest to review. The remaining commits are practically move-only. The only change is creating CMainSignals& GetMainSignals() for accessing g_signals from main. So the PR becomes trivial to review and it’s much less likely to need rebases. On the other hand the cleaning up commit is likely to need frequent rebase and would likely break many open PRs so I’ve created #5697 which builds on top of this as well as on other useful and trivial preparations for consensus and policy code movements (#5696).
  12. jtimon renamed this:
    Refactor: Don't include main.h from any other header
    MOVEONLY-ish: Preparations to not include main.h from any other header
    on Jan 23, 2015
  13. jtimon force-pushed on Feb 3, 2015
  14. jtimon commented at 6:16 pm on February 3, 2015: contributor
    Needed rebase.
  15. jtimon force-pushed on Feb 5, 2015
  16. jtimon renamed this:
    MOVEONLY-ish: Preparations to not include main.h from any other header
    MOVEONLY-ish: Do not include main.h from any other header
    on Feb 5, 2015
  17. jtimon commented at 1:08 am on February 5, 2015: contributor
    Needed rebase. I added a commit to not include main.h from any other header again, but it’s not a big and exhaustive cleanup as it was at the beginning. I still leave that for #5697 after more code movements.
  18. theuni commented at 3:35 am on February 5, 2015: member
    ut ACK, assuming travis is happy tomorrow. Verified that the MOVEONLY commits are still move only.
  19. theuni commented at 7:32 pm on February 19, 2015: member
    ACK. Could you please give this a quick bump for travis, though? There were some caching issues that have been long-fixed.
  20. jtimon force-pushed on Feb 20, 2015
  21. jtimon commented at 3:36 am on February 20, 2015: contributor
    Changed the last commit’s id to give travis another try as demanded by @theuni
  22. theuni commented at 4:20 am on February 20, 2015: member

    s/demanded by/humbly requested by/ :)

    All green now, thanks.

  23. jtimon commented at 7:23 pm on February 20, 2015: contributor
    Sure, humbly requested, thanks for the review and ack.
  24. jtimon commented at 2:11 pm on March 11, 2015: contributor
    ping
  25. sipa commented at 2:14 pm on March 11, 2015: member
    The first commit doesn’t make sense to me: CNodeStateStats is statistics of CNodeState, which is a main-specific data structure. Moving it up to a higher layer (net) seems wrong.
  26. jtimon commented at 4:51 pm on March 11, 2015: contributor
    How is net.o is higher level than main.o? main.cpp depends on net and not the other way around…
  27. sipa commented at 9:06 am on March 12, 2015: member

    Maybe my terminology is confused. I mean main is a client of net, building on top of it. Net should not know anything about what its clients do. CNodeStateStats is stats about CNodeState, the message processing specific data structure about nodes. Message processing is done in main, so this data structure definition belongs there and nowhere else.

    Don’t get me wrong: the current placing of code in main and net is terrible, but we shouldn’t hack around that. Message processing needs to be split up, and moved out to separate modules, and the related data structures with it.

  28. jtimon force-pushed on Mar 13, 2015
  29. jtimon commented at 11:05 am on March 13, 2015: contributor
    Rebased without the first commit as suggested by @sipa
  30. in src/qt/signverifymessagedialog.cpp: in 223eff06a7 outdated 
    11@@ -12,6 +12,7 @@
12 
13 #include "base58.h"
14 #include "init.h"
15+#include "main.h" // For strMessageMagic
    sipa commented at 2:01 pm on March 13, 2015:
    That’s sad, we should move that elsewhere (not a criticism on your PR, just noticing).
  31. sipa commented at 2:03 pm on March 13, 2015: member
    Untested ACK.
  32. theuni commented at 8:28 pm on March 15, 2015: member
    utACK. I’d like to see the node info moved out as well, but that makes sense to do separately.
  33. Includes: Refactor: Move CValidationInterface and CMainSignals out of main 26c16d9de9
  34. Includes: MOVEONLY: move more method definitions out of wallet.h eca0b1ea62
  35. Includes: Do not include main.h from any other header 8a893c949b
  36. jtimon force-pushed on Mar 24, 2015
  37. jtimon commented at 4:50 pm on March 24, 2015: contributor
    Needed rebase, I also slightly improved the includes. The commit that was criticized was already removed before needing the latest rebase. This is quite uncontroversial, it has been reviewed, it has been open for long…is there any specific reason not to merge this?
  38. laanwj commented at 5:06 pm on March 24, 2015: member
    @jtimon No specific reason, just a overload of pulls caused me to lose track of this one for a while, sorry for that.
  39. laanwj merged this on Mar 24, 2015
  40. laanwj closed this on Mar 24, 2015
  41. laanwj referenced this in commit 8d2fbfa491 on Mar 24, 2015
  42. laanwj referenced this in commit cbb2cf5522 on Mar 24, 2015
  43. fanquake referenced this in commit 63e4c9cd35 on Mar 29, 2015
  44. laanwj referenced this in commit d62fed14ab on Mar 30, 2015
  45. MarcoFalke locked this on Sep 8, 2021


jtimon theuni laanwj Diapolo sipa

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 19:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me