refactor: Remove char serialize #22167
Conversation
|
Concept ACK: as we all know by know explicit is better than implicit :) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
fa77984 to
faf9589
Compare
|
Concept ACK |
|
Added a commit to remove MakeUCharSpan where not needed |
| @@ -17,7 +17,7 @@ | |||
| static void DeserializeBlockTest(benchmark::Bench& bench) | |||
| { | |||
| CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION); | |||
| char a = '\0'; | |||
| uint8_t a{'\0'}; | |||
There was a problem hiding this comment.
Nit: for something that looks like an integer type, I find initializing with 0 (uint8_t a{0};) more naturally looking than a char literal.
| @@ -196,7 +195,7 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; } | |||
| FORMATTER_METHODS(cls, obj) | |||
|
|
|||
| #ifndef CHAR_EQUALS_INT8 | |||
| template<typename Stream> inline void Serialize(Stream& s, char a ) { ser_writedata8(s, a); } // TODO Get rid of bare char | |||
| template<typename Stream> void Serialize(Stream&, char) { static_assert(ALWAYS_FALSE<Stream>, "char serialization forbidden use uint8_t or int8_t"); } | |||
There was a problem hiding this comment.
I think this can be written more simply as template<typename Stream> void Serialize(Stream&, char) = delete; (though perhaps with a slightly less clear error message).
|
Concept ACK, though I wonder if this alternative approach isn't better: C++20 If the stream write/read functions were changed to take That needs more invasive changes than what you're aiming for here, but perhaps ones we want to aim for anyway? |
Ok, will do that instead. It will require changing twice as many lines of code, but given that we are starting to use spans consistently, it will likely happen anyway at some point. Combining this Span change with std::byte is "free" (doesn't require a larger diff). |
…alize fa5d2e6 Remove unused char serialize (MarcoFalke) fa24493 Use spans of std::byte in serialize (MarcoFalke) fa65bbf span: Add BytePtr helper (MarcoFalke) Pull request description: This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`). The benefits of using `Span`: * Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization The benefits of using `std::byte`: * `std::byte` can't accidentally be mistaken for an integer The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type. Other changes that are included here: * [#22167](bitcoin/bitcoin#22167) (refactor: Remove char serialize by MarcoFalke) * [#21906](bitcoin/bitcoin#21906) (Preserve const in cast on CTransactionSignatureSerializer by promag) ACKs for top commit: laanwj: Concept and code review ACK fa5d2e6 sipa: re-utACK fa5d2e6 Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
Follow-up to #21969.
Serialize doesn't care whether one of the bits in a
charis a sign bit or not, but having the possibility is slightly confusing to calling code.int8_tanduint8_tcan be used as replacement tochar.