This is the next piece of the puzzle from #10785. It includes:
FORMATTER_METHODS macro, similar to SERIALIZE_METHODS, for defining a formatter with a unified serialization/deserialization implementation.compression.h to consist of 3 formatters, rather than old-style wrappers (ScriptCompression, AmountCompression, TxOutCompression).
https://github.com/bitcoin/bitcoin/blob/fd691da3b032e439936672e084b7c49086ec9ebc/src/coins.h#L59-L65
A signed integer overflow will take place on L62 in case of serialising a coin with a very large nHeight value.
It wasn’t introduced in this PR, but since you’re making changes to the coin serialization/deserialization functions here it might be worth fixing in the same go :)
Also the Unserialize function should probably be adjusted to not accept block_heights above (std::numeric_limits<uint32_t>::max() - 1) / 2.
https://github.com/bitcoin/bitcoin/blob/fd691da3b032e439936672e084b7c49086ec9ebc/src/coins.h#L67-L74
31@@ -32,7 +32,7 @@ class TxInUndoSerializer
32 // Required to maintain compatibility with older undo format.
33 ::Serialize(s, (unsigned char)0);
34 }
35- ::Serialize(s, CTxOutCompressor(REF(txout->out)));
36+ ::Serialize(s, Using<TxOutCompression>(REF(txout->out)));
In commit “Convert compression.h to new serialization framework” (fd691da3b032e439936672e084b7c49086ec9ebc)
Can you drop uses of REF macro here and below?
246+ Unser(s, *this); \
247 } \
248- template<typename Stream, typename Type, typename Operation> \
249- static inline void SerializationOps(Type& obj, Stream& s, Operation ser_action) \
250-
251+ FORMATTER_METHODS(cls, obj)
In commit “Add FORMATTER_METHODS, similar to SERIALIZE_METHODS, but for formatters” (ca34c5cba5fbb9b046b074a234f06ecf6ed5d610)
It’s a little surprising to see SERIALIZE_METHODS depend on FORMATTER_METHODS, but I guess it saves some repetition, and it seems reasonable
I think it makes sense: every class gets the ability to function as a formatter. Both FORMATTER_METHODS and SERIALIZE_METHODS help do that; with the first it’s for an arbitrary type, with the latter it’s for objects of the class it’s being defined it. The latter then also hooks up the Serialize/Unserialize functions to the self-formatter.
Do you suggest documenting this better, or have some other suggestion?
It’s a little surprising to see SERIALIZE_METHODS depend on FORMATTER_METHODS, but I guess it saves some repetition, and it seems reasonable
I think it makes sense: every class gets the ability to function as a formatter. Both FORMATTER_METHODS and SERIALIZE_METHODS help do that; with the first it’s for an arbitrary type, with the latter it’s for objects of the class it’s being defined it. The latter then also hooks up the Serialize/Unserialize functions to the self-formatter.
I think it’s fine. A formatter class with a formatter methods is a class that can serialize and deserialize objects in a particular format. A serializable type with serialize methods is a class whose instances can serialize and deserialize themselves. I wasn’t expecting serializeable objects to now be able to format other objects of the same type, and I couldn’t think of a use for that, but I think it’s harmless.
Do you suggest documenting this better, or have some other suggestion?
I guess the suggestion would be to keep the new FORMATTER_METHODS macro exactly like you have it here, but revert the changes to the SERIALIZE_METHODS macro, so it isn’t affected by this PR. But only if you want to have a stronger distinction between formatter classes and serializable types. I’m fine either way.
