Skip to content

refactor: [index] Replace deprecated char with uint8_t in serialization #21824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 5, 2021
Merged

refactor: [index] Replace deprecated char with uint8_t in serialization #21824

merged 1 commit into from
May 5, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented May 1, 2021

All char representations are serialized in the same way, however the char one is deprecated according to

bitcoin/src/serialize.h

Line 227 in d22e7ee

template<typename Stream> inline void Serialize(Stream& s, char a ) { ser_writedata8(s, a); } // TODO Get rid of bare char
. Also, using uint8_t directly avoids casts.

@jonatack
Copy link
Member

jonatack commented May 1, 2021

Nice change.

Approach ACK fafb880

@kristapsk
Copy link
Contributor

Concept ACK, char is bad in C++ in general for both it's undefined default signedness and a lot of people thinking it's garanteed to be 8-bit, which is not true.

Haven't yet looked though all of the code, but shouldn't char versions of Serialize()/Unserialize() in src/serialize.h be removed too?

@practicalswift
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented May 4, 2021

Code review ACK fafb880
Using explicitly signed/unsigned sized types is clearly superior for serialization purposes

@practicalswift
Copy link
Contributor

cr ACK fafb880: patch looks correct

uint8_t and uniform initialisation {} make things significantly easier to reason about. Thanks!

@fanquake fanquake merged commit dc8da2a into bitcoin:master May 5, 2021
@maflcko maflcko deleted the 2105-refactorUint8_t branch May 5, 2021 16:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2021
@fanquake
Merge bitcoin#21824: refactor: [index] Replace deprecated char with u…
24dc4f4 
…int8_t in serialization

fafb880 refactor: [index] Replace deprecated char with uint8_t in serialization (MarcoFalke)

Pull request description:

  All char representations are serialized in the same way, however the `char` one is deprecated according to https://github.com/bitcoin/bitcoin/blob/d22e7ee93313b13365bd14a5fffeb055cff4dcd2/src/serialize.h#L227 . Also, using `uint8_t` directly avoids casts.

ACKs for top commit:
  jonatack:
    Approach ACK fafb880
  laanwj:
    Code review ACK fafb880
  practicalswift:
    cr ACK fafb880: patch looks correct

Tree-SHA512: ed08fb1b18cb75a695e15924bcaa30ff8746bcd5f17cc83e79f94fe5ff8d9f2083435cb49b8245e3341ede2512140940d864299f4746bc40c8ed8bfdbdacac24
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge dc8da2a into merged_master (Bitcoin PR bitcoin/bitcoin#21824)
44f5847
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Refactoring UTXO Db and Indexes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maflcko @jonatack @kristapsk @practicalswift @laanwj @fanquake @DrahtBot