Net: Trivial-review: Make SendMessages easier to review #9579
Conversation
src/net_processing.cpp
Outdated
| @@ -2681,6 +2681,9 @@ class CompareInvMempoolOrder | |||
| } | |||
| }; | |||
|
|
|||
| void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch); | |||
There was a problem hiding this comment.
Maybe move the declarations (not definitions) to the header?
There was a problem hiding this comment.
The idea was to leave them as static rather than expose them (unless someone else wants to expose some of them later). Perhaps I should have started from the top to avoid the extra declarations. No big deal for me to change it if we decide to divide the whole function ala #9579
src/net_processing.cpp
Outdated
| @@ -2681,6 +2681,9 @@ class CompareInvMempoolOrder | |||
| } | |||
| }; | |||
|
|
|||
| void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch); | |||
| void MsgFeeFilter(CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker); | |||
There was a problem hiding this comment.
I think CNetMsgMaker::Make could be const, which might be worth doing to be able to make this reference const.
There was a problem hiding this comment.
No, CNetMsgMaker::Make() is not const :(
There was a problem hiding this comment.
I see no reason why it couldn't be const.
There was a problem hiding this comment.
Right, I didn't realise both versions of CNetMsgMaker::Make can be just made const too. Very good catch.
Updating both this and #9608.
src/net_processing.cpp
Outdated
| @@ -3116,7 +3119,14 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg | |||
| return true; | |||
| } | |||
| } | |||
| MsgGetData(consensusParams, pto, connman, msgMaker, state, nNow, fFetch); | |||
There was a problem hiding this comment.
Re nNow - Can't it work out the time by itself?
|
I would advice reviewers to look at #9608 first, which I should have written first. |
b319469 to
bb41dab
Compare
bb41dab to
b2d05fa
Compare
|
Rebased on top of #9659 which separates @JeremyRubin 's suggestion and includes a few more similar things. |
575aa24 to
ab4f51e
Compare
|
Rebased after #9659 has been merged for clarity. |
src/net_processing.cpp
Outdated
|
|
||
| void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, const CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch) |
There was a problem hiding this comment.
Maybe make nNow and fFetch const, to ensure they are not mistakenly updated (and discarded on return) in this method.
There was a problem hiding this comment.
Long time ago, I remember I was asked not to do this for basic types. Recently we discussed a PR to change this in the whole codebase (sorry, I can't find it). But good good point, no reason not to do it here and in #9608. It also helps me read functions when more parameters are const.
ab4f51e to
2e4b085
Compare
|
Fixed nits ( #9608 (comment) #9579 (comment) ) and rebased without needing it. |
2e4b085 to
d737d1f
Compare
More things to do later with more disruption: - The new functions can be turned into static unless someone else has a better idea - The documentation should be moved to doxygen - An indentation can be done for each of the new function - MsgGetData() could be divided in 2, but I don't know if it makes sense
d737d1f to
b2a2ba5
Compare
|
Rebased just because travis couldn't fetch a file. |
|
Needed rebase, closing. |
More things to do later with more disruption:
In the spirit of #5153, happy to go further dividing SendMessages in the low disruption territory, rem +13-3