Encapsulate & protect members of CMessageHeader (kinda), CInv, CAddress #4661

pull jgarzik wants to merge 6 commits into bitcoin:master from jgarzik:2014_encaps_proto changing 6 files +70 −60

jgarzik commented at 4:07 PM on August 8, 2014: contributor

CMessageHeader is directly accessed, so the conversion was merely done for non-direct-access callsites.
jgarzik added the label Improvement on Aug 8, 2014
jgarzik added the label Priority Low on Aug 8, 2014
in src/protocol.h:None in 81586c6d94 outdated
```
 139 | @@ -131,9 +140,10 @@ class CInv
 140 |          const char* GetCommand() const;
 141 |          std::string ToString() const;
 142 |          void print() const;
 143 | +        int GetType() const { return type; }
 144 | +        uint256 GetHash() const { return hash; }
 145 |  
 146 | -    // TODO: make private (improves encapsulation)
 147 | -    public:
 148 | +    protected:
```
theuni commented at 7:54 PM on August 8, 2014:

This can be private

jgarzik commented at 7:43 PM on August 11, 2014:

Can be, yes. But if this class is sub-classed in the future, that would need to change.

Seems more correct, if unnecessary at present, to use "protected" unless there is a specific reason to use "private". These members are key to the data structure, and seem likely to be wanted if a subclass ever arises.

in src/protocol.h:None in 81586c6d94 outdated

 139 | @@ -131,9 +140,10 @@ class CInv
 140 |          const char* GetCommand() const;
 141 |          std::string ToString() const;
 142 |          void print() const;
 143 | +        int GetType() const { return type; }
 144 | +        uint256 GetHash() const { return hash; }

laanwj commented at 2:22 PM on August 21, 2014:

const uint256 &GetHash() const to avoid a copy?

laanwj commented at 2:24 PM on August 21, 2014: member

Looks good to me, I think we should do this for the network code, but not the consensus code
TheBlueMatt commented at 3:21 AM on August 22, 2014: member

@laanwj That sentiment seems reasonable, the code certainly looks cleaner on CMessageHeader, though I'm not sold on it for CInv, that one seems reasonable as a POD non-OO thing as well, though I dont feel incredibly strongly about that.
Encapsulate class CAddress public members. de41b63e3f
Encapsulate accesses to CMessageHeader
Also, improve GetCommand()
077edc3cd9
Encapsulate members of class CInv 7f361f2d18
protocol.h: include string.h for strnlen(3) e132d27070
Back out improved GetCommand() Windows does not support strnlen() ef17fbc4df
CInv: return const hash ref to av oid copy 9803b7a4b1
jgarzik force-pushed on Aug 25, 2014
BitcoinPullTester commented at 7:35 PM on August 25, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4661_9803b7a4b18c9f9f94a98ec2389ebccd3263d8f3/ 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.
jtimon commented at 1:27 PM on June 21, 2015: contributor

Rebase, replace or close?
jgarzik commented at 6:04 PM on July 23, 2015: contributor

Closing old PR. no ACKs after a while.

Still think it's a useful change, but not worth continually rebasing a refactor if no ACKs.
jgarzik closed this on Jul 23, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels