refactor: Remove char serialize #22167

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

2. fanquake added the label Refactoring on Jun 6, 2021
3. 
   practicalswift commented at 11:10 am on June 6, 2021: contributor

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

   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.

6. 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.
7. 
   Empact commented at 4:14 am on June 7, 2021: member

   Concept ACK
8. MarcoFalke force-pushed on Jun 7, 2021
9. MarcoFalke force-pushed on Jun 7, 2021
10. MarcoFalke force-pushed on Jun 7, 2021
11. 
    theStack commented at 2:38 pm on June 18, 2021: member

    Concept ACK
12. MarcoFalke force-pushed on Jul 26, 2021
13. MarcoFalke force-pushed on Aug 30, 2021
14. 
    promag commented at 1:07 pm on August 31, 2021: member

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

    Added a commit to remove MakeUCharSpan where not needed
21. 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.
22. 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).
23. 
    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?

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

25. MarcoFalke closed this on Nov 4, 2021

    ---

26. MarcoFalke deleted the branch on Nov 4, 2021
27. laanwj referenced this in commit 196b459920 on Jan 27, 2022
28. DrahtBot locked this on Nov 4, 2022

Contributors
MarcoFalke practicalswift DrahtBot Empact theStack promag sipa

Labels
Refactoring