refactor: Switch serialize to uint8_t (Bundle 1/2) #21969

1. 
   MarcoFalke commented at 5:57 am on May 17, 2021: member

   Replace char -> uint8_t in serialization where a sign doesn’t make sense (char might be signed/unsigned).
2. fanquake added the label Refactoring on May 17, 2021
3. 
   practicalswift commented at 8:32 am on May 17, 2021: contributor

   Concept ACK

   Explicit is better than implicit.

4. in src/addrman.cpp:39 in bbbbad6e20 outdated

   33@@ -34,7 +34,7 @@ int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std:
   34 
   35 int CAddrInfo::GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const
   36 {
   37-    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? 'N' : 'K') << nBucket << GetKey()).GetCheapHash();
   38+    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << nBucket << GetKey()).GetCheapHash();
   

   ---

   ---

   

   hebasto commented at 8:48 am on May 17, 2021:

   Does this involve a conversion from the char type?

   ---

   

   MarcoFalke commented at 11:43 am on May 17, 2021:

   Not sure what you mean. This tells the compiler to select the uint8_t-serialization for the ASCII-character N (or K).

   None of the ASCII characters use the “sign”-bit, so the serialization of them is identical, regardless of whether they are serialized as int8_t, char, unsinged char, uint8_t, or signed char

   ---

   

   hebasto commented at 12:02 pm on May 17, 2021:

   I mean that char literals have type char. And it is not clear for me if uint8_t{'N'} involves any type conversion?

   None of the ASCII characters use the “sign”-bit…

   Right.

   ---

   

   MarcoFalke commented at 7:47 am on May 20, 2021:

   Is this a philosophical question?

   Previously the compiler produced code to serialize the value 78 (which is ‘N’) by selecting the char serialization template. Now, the compiler produces code to serialize the value 78 (which is also ‘N’) by selecting the uint8_t serialization template.

   Both templates map to the same code, so the binary should be identical. (Modulo some debug symbols, maybe)

   ---

   

   MarcoFalke commented at 8:01 am on May 20, 2021:

   On my system this line of the patch doesn’t change the binaries for gcc and clang with -O2.

   ---

   

   laanwj commented at 9:40 am on May 27, 2021:

   It would have been nice to have constants for these instead of hardcoding them, for documentation purposes, but the change looks fine to me.
5. 
   kristapsk commented at 10:32 am on May 17, 2021: contributor

   Concept ACK
6. 
   DrahtBot commented at 2:53 am on May 19, 2021: member

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

   Conflicts

   No conflicts as of last run.

7. 
   theStack commented at 3:07 pm on May 24, 2021: member

   Concept ACK
8. DrahtBot added the label Needs rebase on May 26, 2021
9. 
   laanwj commented at 9:40 am on May 27, 2021: member

   Code review ACK bbbbad6e20cece7fa2884737c223e10b2b0d7066
10. refactor: Switch serialize to uint8_t (1/n) ffff0d0442
11. MarcoFalke force-pushed on May 31, 2021
12. DrahtBot removed the label Needs rebase on May 31, 2021
13. 
    practicalswift commented at 1:11 pm on May 31, 2021: contributor

    cr ACK ffff0d04425a616c14fc4a562e8ef93d286705f8: patch looks correct and commit hash is ffffresh (was bbbbadass)
14. kristapsk approved
15. 
    kristapsk commented at 3:08 pm on May 31, 2021: contributor

    ACK ffff0d04425a616c14fc4a562e8ef93d286705f8
16. MarcoFalke merged this on Jun 1, 2021
17. MarcoFalke closed this on Jun 1, 2021

    ---

18. MarcoFalke deleted the branch on Jun 1, 2021
19. sidhujag referenced this in commit 579436d522 on Jun 1, 2021
20. gwillen referenced this in commit f39648bfe5 on Jun 1, 2022
21. DrahtBot locked this on Aug 16, 2022

Contributors
MarcoFalke practicalswift hebasto laanwj kristapsk DrahtBot theStack

Labels
Refactoring