This moves all the header sizes together. Also move the enum to the top, and remove a deceptive TODO comment (this is just a dumb data structure describing the protocol and there's nothing to be gained from encapsulation at that level).
Other commit fixes a silly typo in version.h
Also move the enum to the top, and remove a deceptive TODO
comment.
utACK
utACK
55 | @@ -48,17 +56,6 @@ class CMessageHeader 56 | READWRITE(FLATDATA(pchChecksum)); 57 | } 58 | 59 | - // TODO: make private (improves encapsulation)
Comment removed. Is it OK?
See OP
and remove a deceptive TODO comment (this is just a dumb data structure describing the protocol and there's nothing to be gained from encapsulation at that level).
utACK
utACK
26 | @@ -29,6 +27,16 @@ 27 | class CMessageHeader 28 | { 29 | public: 30 | + enum { 31 | + MESSAGE_START_SIZE = 4,
nit: could make these constexprs instead while we're at it. No real reason other than cosmetic, though.
Huh yes now I look at it again using an enum here is a somewhat of a strange use of the C++ language.
Though it looks like we use this kind of constant declaration in many places, and so does leveldb:
arith_uint256.h: enum { WIDTH=BITS/32 };
chain.h: enum { nMedianTimeSpan=11 };
leveldb/db/skiplist.h: enum { kMaxHeight = 12 };
leveldb/helpers/memenv/memenv.cc: enum { kBlockSize = 8 * 1024 };
...
So I suppose it's somewhat acceptable in some cases?
@laanwj Looks like several of those are hold-overs from c++03. In the case where there's no actual grouping and we're just using them as constants, I think constexpr's would be fine, but the enum is ofc acceptable as well.
Nevermind for here, we can discuss in a separate style guide PR.
utACK 2c09a5209ab00573a2422e1e65c437a6e2f59624
Added another commit:
This concretizes the numbers and adds a comment to make it clear that
these numbers are fixed by the protocol, and may avoid people forgetting
to claim numbers in the future (e.g. issue #8500).
Also gets rid of a weird unused `MSG_TYPE_MAX` in the middle of the
enumeration (thanks @paveljanik for noticing).
326 | UNDEFINED = 0, 327 | - MSG_TX, 328 | - MSG_BLOCK, 329 | + MSG_TX = 1, 330 | + MSG_BLOCK = 2, 331 | MSG_TYPE_MAX = MSG_BLOCK,
MSG_TYPE_MAX is not used at all in the sources and looks strange in the middle...
HUH. That's a good catch. How could I have missed that one.
Got rid of the awkward MSG_TYPE_MAX leftover in between the enum values (and squashed). Thanks @paveljanik for noticing.
The MSG_TYPE_MAX is a leftover from earlier code that got refactored in the segwit PR. It delimited the maximum msg type that could be seen in an inv (as opposed to the next ones, which can only be seen in getdata).
The MSG_TYPE_MAX is a leftover from earlier code that got refactored in thesegwit PR.
OK, in historical context it makes sense, it just looks weird now.
utACK 1df311118d79c04df1d41e044b19444cfda015da