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

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2105-uin8t changing 8 files +45 −49

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).
fanquake added the label Refactoring on May 17, 2021
practicalswift commented at 8:32 AM on May 17, 2021: contributor

Concept ACK

Explicit is better than implicit.
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.
kristapsk commented at 10:32 AM on May 17, 2021: contributor

Concept ACK
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.
theStack commented at 3:07 PM on May 24, 2021: member

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

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

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

ACK ffff0d04425a616c14fc4a562e8ef93d286705f8
MarcoFalke merged this on Jun 1, 2021
MarcoFalke closed this on Jun 1, 2021
MarcoFalke deleted the branch on Jun 1, 2021
sidhujag referenced this in commit 579436d522 on Jun 1, 2021
gwillen referenced this in commit f39648bfe5 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022