Break up CAddrMan's IMPLEMENT_SERIALIZE #4508

pull sipa wants to merge 1 commits into bitcoin:master from sipa:addrserial changing 2 files +160 −129

sipa commented at 6:19 PM on July 10, 2014: member

clang-format cannot deal with it, and, admittedly, it's way too big anyway.

In order not to duplicate the writing logic for computing the size, introduce a CSizeComputer in serialize.h: a minimal serializer stream implementation that only computes the number of bytes written. Due to inlining, this should be as efficient as the existing GetSerializeSize code.

I'm thinking about getting rid of IMPLEMENT_SERIALIZE entirely, and replace it with one generic method that depending on template instantiation can do either read/write/size.
sipa commented at 7:33 PM on July 10, 2014: member

Tested: waiting until a peers.dat write succeeds, restart bitcoin, see reading addresses succeeds.
jaromil cross-referenced this on Jul 11, 2014 from issue Proof of work related refactor by jtimon
sipa commented at 4:58 PM on July 16, 2014: member

Can I have some ACKs, so I can update #4498 with it? @laanwj @gavinandresen @jgarzik @gmaxwell ?
jgarzik commented at 6:21 PM on July 16, 2014: contributor

quick it-works test. ACK.

nit: a //comment or commit msg txt explaining lack of IMPLEMENT_SERIALIZE(). Your pull request description covered that, but PRs aren't really part of the git history.

in src/serialize.h:None in fded96db71 outdated

 837 | +
 838 | +public:
 839 | +    int nType;
 840 | +    int nVersion;
 841 | +
 842 | +    CSizeComputer(int nTypeIn, int nVersionIn) : nType(nTypeIn), nVersion(nVersionIn) {}

gavinandresen commented at 6:23 PM on July 16, 2014:

Need to initialize nSize to zero here...

gavinandresen commented at 6:27 PM on July 16, 2014: contributor

Code review ACK except for init CSizeComputer::nSize comment.
Break up CAddrMan's IMPLEMENT_SERIALIZE b069750d3f
sipa commented at 7:17 PM on July 16, 2014: member

Fixed.
BitcoinPullTester commented at 7:41 PM on July 16, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4508_b069750d3f27c96a83700a08a2bb819902268857/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa merged this on Jul 17, 2014
sipa closed this on Jul 17, 2014
sipa referenced this in commit fb203cab40 on Jul 17, 2014
sipa cross-referenced this on Jul 17, 2014 from issue Coding style update with clang-format by sipa
bitcoin locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4377 Proof of work related refactor #4498 Coding style update with clang-format