refactor: Remove char serialize #22167

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2105-uin8t changing 23 files +109 −114

MarcoFalke commented at 9:59 AM on June 6, 2021: member

Follow-up to #21969.

Serialize doesn't care whether one of the bits in a char is a sign bit or not, but having the possibility is slightly confusing to calling code. int8_t and uint8_t can be used as replacement to char.
fanquake added the label Refactoring on Jun 6, 2021
practicalswift commented at 11:10 AM on June 6, 2021: contributor

Concept ACK: as we all know by know explicit is better than implicit :)
MarcoFalke force-pushed on Jun 6, 2021
DrahtBot commented at 2:34 PM on June 6, 2021: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22981 (doc: Fix incorrect C++ named args by MarcoFalke)
- #19690 (util: improve FindByte() performance by LarryRuane)
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.
MarcoFalke cross-referenced this on Jun 6, 2021 from issue refactor: Preserve const in cast on CTransactionSignatureSerializer by promag
DrahtBot cross-referenced this on Jun 6, 2021 from issue util: improve FindByte() performance by LarryRuane

in src/serialize.h:62 in fa2e0230f1 outdated

  61 |   * Lowest-level serialization and conversion.
  62 |   * @note Sizes of these types are verified in the tests
  63 |   */
  64 |  template<typename Stream> inline void ser_writedata8(Stream &s, uint8_t obj)
  65 |  {
  66 | -    s.write((char*)&obj, 1);

Empact commented at 4:12 AM on June 7, 2021:

nit: Uint8Ptr unnecessary as this is already uint8_t*

MarcoFalke commented at 7:08 AM on June 7, 2021:

I think I'll keep this as is for symmetry with the other helpers.

Empact commented at 4:14 AM on June 7, 2021: member

Concept ACK
MarcoFalke force-pushed on Jun 7, 2021
MarcoFalke force-pushed on Jun 7, 2021
MarcoFalke force-pushed on Jun 7, 2021
theStack commented at 2:38 PM on June 18, 2021: contributor

Concept ACK
MarcoFalke force-pushed on Jul 26, 2021
MarcoFalke force-pushed on Aug 30, 2021
promag commented at 1:07 PM on August 31, 2021: member

Concept ACK.
MarcoFalke force-pushed on Sep 9, 2021
MarcoFalke force-pushed on Sep 29, 2021
refactor: Remove char serialize fa4238e70f
Remove MakeUCharSpan where not needed fa1db77edf
MarcoFalke force-pushed on Nov 2, 2021
MarcoFalke commented at 4:03 PM on November 2, 2021: member

Added a commit to remove MakeUCharSpan where not needed

in src/bench/checkblock.cpp:20 in fa4238e70f outdated

  16 | @@ -17,7 +17,7 @@
  17 |  static void DeserializeBlockTest(benchmark::Bench& bench)
  18 |  {
  19 |      CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
  20 | -    char a = '\0';
  21 | +    uint8_t a{'\0'};

sipa commented at 4:08 PM on November 2, 2021:

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.

in src/serialize.h:198 in fa4238e70f outdated

 194 | @@ -196,7 +195,7 @@ template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
 195 |      FORMATTER_METHODS(cls, obj)
 196 |  
 197 |  #ifndef CHAR_EQUALS_INT8
 198 | -template<typename Stream> inline void Serialize(Stream& s, char a    ) { ser_writedata8(s, a); } // TODO Get rid of bare char
 199 | +template<typename Stream> void Serialize(Stream&, char) { static_assert(ALWAYS_FALSE<Stream>, "char serialization forbidden use uint8_t or int8_t"); }

sipa commented at 4:13 PM on November 2, 2021:

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).

sipa commented at 4:22 PM on November 2, 2021: member

Concept ACK, though I wonder if this alternative approach isn't better:

C++20 std::span has std::as_bytes and std::as_writable_bytes functions to convert spans to equivalent spans-to-byte-representation. While we don't have std::byte yet, we could introduce equivalent operations that use unsigned char instead.

If the stream write/read functions were changed to take Span<(const) unsigned char>, many of the cases here could very naturally be written as s.write(AsWritableUChar(Span{arg})); for example.

That needs more invasive changes than what you're aiming for here, but perhaps ones we want to aim for anyway?
DrahtBot cross-referenced this on Nov 3, 2021 from issue doc: Fix incorrect C++ named args by MarcoFalke
MarcoFalke commented at 1:32 PM on November 4, 2021: member

If the stream write/read functions were changed to take Span<(const) unsigned char>, many of the cases here could very naturally be written as s.write(AsWritableUChar(Span{arg})); for example.

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).
MarcoFalke closed this on Nov 4, 2021
MarcoFalke deleted the branch on Nov 4, 2021
MarcoFalke cross-referenced this on Nov 4, 2021 from issue refactor: Use spans of std::byte in serialize by MarcoFalke
laanwj referenced this in commit 196b459920 on Jan 27, 2022
MarcoFalke cross-referenced this on Jan 31, 2022 from issue refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta
bitcoin locked this on Nov 4, 2022

Contributors

Labels

Linked (view graph)

#21906 refactor: Preserve const in cast on CTransactionSignatureSerializer #21969 refactor: Switch serialize to uint8_t (Bundle 1/2)#23438 refactor: Use spans of std::byte in serialize #24185 refactor: only use explicit reinterpret/const casts, not implicit

refactor: Remove char serialize #22167

Conflicts