Net: Trivial-review: Make SendMessages easier to review

jtimon commented at 3:07 am on January 19, 2017: contributor

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

In the spirit of #5153, happy to go further dividing SendMessages in the low disruption territory, rem +13-3

in src/net_processing.cpp: in b319469bf5 outdated

2680@@ -2681,6 +2681,9 @@ class CompareInvMempoolOrder
2681     }
2682 };
2683 
2684+void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch);

JeremyRubin commented at 4:16 am on January 19, 2017:

Maybe move the declarations (not definitions) to the header?

jtimon commented at 9:16 am on January 21, 2017:

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

in src/net_processing.cpp: in b319469bf5 outdated

2680@@ -2681,6 +2681,9 @@ class CompareInvMempoolOrder
2681     }
2682 };
2683 
2684+void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch);
2685+void MsgFeeFilter(CNode* pto, CConnman& connman, CNetMsgMaker& msgMaker);

JeremyRubin commented at 4:22 am on January 19, 2017:

I think CNetMsgMaker::Make could be const, which might be worth doing to be able to make this reference const.

jtimon commented at 0:16 am on January 24, 2017:

No, CNetMsgMaker::Make() is not const :(

sipa commented at 0:18 am on January 24, 2017:

I see no reason why it couldn’t be const.

jtimon commented at 0:41 am on January 24, 2017:

Right, I didn’t realise both versions of CNetMsgMaker::Make can be just made const too. Very good catch. Updating both this and #9608.

JeremyRubin commented at 4:24 am on January 19, 2017: contributor

Concept ACK

fanquake added the label P2P on Jan 19, 2017

dcousens approved

in src/net_processing.cpp: in b319469bf5 outdated

3118@@ -3116,7 +3119,14 @@ bool SendMessages(CNode* pto, CConnman& connman, std::atomic<bool>& interruptMsg
3119                 return true;
3120             }
3121         }
3122+        MsgGetData(consensusParams, pto, connman, msgMaker, state, nNow, fFetch);

rebroad commented at 7:26 am on January 23, 2017:

Re nNow - Can’t it work out the time by itself?

jtimon commented at 0:18 am on January 24, 2017: contributor

I would advice reviewers to look at #9608 first, which I should have written first.

jtimon force-pushed on Jan 24, 2017

jtimon force-pushed on Jan 31, 2017

jtimon commented at 11:05 pm on January 31, 2017: contributor

Rebased on top of #9659 which separates @JeremyRubin ’s suggestion and includes a few more similar things.

jtimon force-pushed on Feb 1, 2017

jtimon force-pushed on Feb 7, 2017

jtimon commented at 8:42 pm on February 7, 2017: contributor

Rebased after #9659 has been merged for clarity.

in src/net_processing.cpp: in ab4f51e89a outdated

3171+        MsgFeeFilter(pto, connman, msgMaker);
3172+    }
3173+    return true;
3174+}
3175 
3176+void MsgGetData(const Consensus::Params& consensusParams, CNode* pto, CConnman& connman, const CNetMsgMaker& msgMaker, CNodeState& state, int64_t nNow, bool fFetch)

kallewoof commented at 1:07 am on March 1, 2017:

Maybe make nNow and fFetch const, to ensure they are not mistakenly updated (and discarded on return) in this method.

jtimon commented at 10:20 pm on March 6, 2017:

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.

kallewoof commented at 1:12 am on March 1, 2017: member

utACK ab4f51e

jtimon force-pushed on Mar 6, 2017

jtimon commented at 10:31 pm on March 6, 2017: contributor

Fixed nits ( #9608 (comment) #9579 (review) ) and rebased without needing it.

jtimon force-pushed on Apr 4, 2017

Net: Trivial-review: Make SendMessages easier to review

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

b2a2ba5b78

jtimon force-pushed on Apr 5, 2017

jtimon commented at 11:01 pm on April 5, 2017: contributor

Rebased just because travis couldn’t fetch a file.

jtimon commented at 0:40 am on June 8, 2017: contributor

Needed rebase, closing.

jtimon closed this on Jun 8, 2017

DrahtBot locked this on Sep 8, 2021

Net: Trivial-review: Make SendMessages easier to review #9579