Remove repeated code and data from Masternode classes #1572
Conversation
* Remove un-needed #includes (what is the policy?) Data was duplicated in masternode_info_t and CMasternode classes: * CMasternode is changed to inherit from masternode_info_t so the data members are inherited rather than repeated (also inherits unrepeated nTimeLastPing and fInfoValid members; this slight intrusiveness made up for in simplicity). * Use in-class member initializers (C++11) for defaults, so only non-default initializers are required in the lists. Allows to shorten repetitous constructor initializer lists. This makes checking for uninitialized data simpler. * Default constructors are defined as "= default;" if possible. * masternode_info_t is changed to behave like an aggregate (but requires over-complicated constructors until c++14). There are pros and cons here - aggregate initialization is convenient but implicit). * Removed user-defined swap functions. They appear to only be used in operator= definitions, using the copy-in,swap-and-return idiom: * Default operator=, where possible. * Move in class `friend bool operator==` out-of-class.
| nPoSeBanHeight(0), | ||
| fAllowMixingTx(true), | ||
| fUnitTest(false) | ||
| masternode_info_t{ MASTERNODE_ENABLED, PROTOCOL_VERSION, GetAdjustedTime()}, |
There was a problem hiding this comment.
It is easy to make error in such constructor. If in the future somebody adds new member or rearrange existing members, he can miss the fixing of the initialization order in ctor. Explicit member initialization is much safer, I think.
There was a problem hiding this comment.
Agree - as noted in the commit message, there are pros and cons here.
The existing explicit lists were long and separated from the data itself.
This duplication had already introduced some initialisation errors.
The (pseudo) aggregate initialization is very open to future error but that can be mitigated.
Stronger typing - if the data member fields had proper types then they could not be initialised out of order - the int constants should be enum types and we should use a time specific type rather than uint64_t. I can make those changes, though it will involve touching more files
There was a problem hiding this comment.
I don't know what is the best way to do it, there are different approaches. Current code doesn't contain errors, and we need more new c++ using in our code, it makes it more simple and readable. But I don't sure about this initializer list ctors.)
| return false; | ||
| } | ||
| if (!mnbRet.Sign(keyCollateralAddressNew)) | ||
| return Log(strprintf("Failed to sign broadcast, masternode=%s", txin.prevout.ToStringShort())); |
There was a problem hiding this comment.
We have lost mnbRet invalidation here. I think, it can be dangerous (it's enough to call Relay on this invalid object to get problems). Current use cases process this situation correctly with Create's returned result, but in the future it can be the reason of some errors.
There was a problem hiding this comment.
I think that the behaviour is the same as before.
The mnbRet = CMasternodeBroadcast() is included in the Log local (lambda) function
(I guess that's what you mean by invalidation).
This change adds a small benefit - it makes it slightly easier to check the cases when we do come to refactor properly. The local function cuts out a bit of repetition, and so compresses the code to scan for the cases, at the expense of having to check what the function is doing.
Really, though, I want to get rid of these Create functions and replace with regular constructors, adding a flag in the class for 'invalid' state if needed. Do you think that is feasible, or is there a need for a separate Create function I'm missing?
There was a problem hiding this comment.
Sorry, it's my error, I've missed the Log lambda contains it.)
…1572) * Remove un-needed #includes (what is the policy?) Data was duplicated in masternode_info_t and CMasternode classes: * CMasternode is changed to inherit from masternode_info_t so the data members are inherited rather than repeated (also inherits unrepeated nTimeLastPing and fInfoValid members; this slight intrusiveness made up for in simplicity). * Use in-class member initializers (C++11) for defaults, so only non-default initializers are required in the lists. Allows to shorten repetitous constructor initializer lists. This makes checking for uninitialized data simpler. * Default constructors are defined as "= default;" if possible. * masternode_info_t is changed to behave like an aggregate (but requires over-complicated constructors until c++14). There are pros and cons here - aggregate initialization is convenient but implicit). * Removed user-defined swap functions. They appear to only be used in operator= definitions, using the copy-in,swap-and-return idiom: * Default operator=, where possible. * Move in class `friend bool operator==` out-of-class.
This PR aims to reduce repetition without changing behaviour.
Comes under trac#6: Improve maintainability of dash code base
Data was duplicated in
masternode_info_tandCMasternodeclasses.Constructors initializer lists were over-long and repetitious.
I already found divergence and uninitialized data due to repetition.
Please check carefully that nothing necessary has been cut,
and no behaviour changed (except for bad behaviour!).