consensus lib work: Reduce boost dependencies#5082
Conversation
src/walletdb.cpp
Outdated
There was a problem hiding this comment.
I don't really like data structures that know their own key in a database, as it breaks composability.
Can you leave the "acentry" and "destdata" out, and leave those in the actual db write methods using a make_pair("acentry", blah)?
|
Concept ACK, and most code changes look good, apart from the nits above. |
|
Nits addressed. |
src/walletdb.cpp
Outdated
There was a problem hiding this comment.
Is this our coding style? m_?
There was a problem hiding this comment.
Old habit for member vars (a good habit for once, imo). I haven't seen any real continuity in the codebase. What would you prefer?
There was a problem hiding this comment.
A long, long time ago
In a galaxy far, far away...
^H^H^H^H^H^H^Hit was always typeVarName, but anything that isnt introducing another conflicting style is good with me (generally matching what is in surrounding code is best).
There was a problem hiding this comment.
I agree that m_ for members is a fairly good practice, but this (some obscure wallet structure) isn't the place to get it started.
Edit: also we always use explicit public: / private:, and usually start the class with public: members/methods.
There was a problem hiding this comment.
But for something slightly more interesting than style: CAccEntryMeta and CDestDataMeta are created, serialized, then deleted again.
They are never deserialized, and the fields are never read.
Do you intend to use these on the read-side of things as well, or are you saving that for a later pull?
There was a problem hiding this comment.
They're being deserialized field by field in the ReadKeyValue function.
|
Mostly looks good...while you're changing serialize.h, can you add comments, as the changes now make it even less clear what is going on in that mess. |
|
@theuni passing the name of the field to the classes is not what I meant; this still means they're not composable. What I meant is: Write(make_pair(std::string("destdata"), CDestData(address, key)), value); However, if you see it like that, there is not even a need for a separate class (though it may help for readability) - you could just as wel use: Write(make_pair(std::string("destdata"), make_pair(address, key)), value); |
|
nits addressed, I believe. I used @sipa's idea to use a pair of pairs for the serialization rather than the custom classes. That seems more clear, and matches one of the uses that was already there as well. I'll squash if all looks good now. |
|
ut ACK |
|
utACK |
|
Yes, not introducing those classes at all is better. utACK |
theuni
force-pushed
the
reduce-boost-dependencies
branch
from
5f1a474 to
5f4bcf6
Compare
|
squashed and rebased. |
There's only one case where a vector containing a fundamental type is serialized all-at-once, unsigned char. Anything else would lead to strange results. Use a dummy argument to overload in that case.
There's only one user of this form of serialization, so it can be easily dropped. It could be re-added if desired when we switch to c++11.
This allows CECKey to be used without directly depending on the secure allocators
Also add a test to verify.
5f4bcf6 boost: drop boost dependency in version.cpp. (Cory Fields) 352058e boost: drop boost dependency in utilstrencodings.cpp (Cory Fields) e1c9467 boost: drop boost dependency in core.cpp (Cory Fields) e405aa4 boost: remove CPrivKey dependency from CECKey (Cory Fields) 5295506 boost: drop dependency on tuple in serialization (Cory Fields) 1d9b86d boost: drop dependency on is_fundamental in serialization (Cory Fields)
4 participants
These are the functional changes needed to remove boost as a dependency from the upcoming consensus lib.
After these changes, a good bit of code movement is still needed to actually remove the dependencies, but it's little more than copy/paste. Those will come as a 2nd PR after this one to ease review.
Please pay special attention to the new serializers in walletdb.cpp. I believe I've ported that correctly, but I'd like to have lots of eyes on it.