This adds the hex->std::byte helper after the std::byte->hex helper was added in commit 9394964f6b9d1cf1220a4eca17ba18dc49ae876d
util: Add ParseHex<std::byte>() helper #23595
pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2111-utilHexByte changing 8 files +28 −13-
MarcoFalke commented at 4:05 PM on November 25, 2021: member
- MarcoFalke added the label Refactoring on Nov 25, 2021
-
DrahtBot commented at 7:56 AM on November 26, 2021: member
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
No conflicts as of last run.
-
laanwj commented at 3:21 PM on November 28, 2021: member
The long-term idea is to move from
uint8_ttostd::byteeverywhere? -
sipa commented at 4:06 PM on November 28, 2021: member
I believe that the rationale behind
std::bytein the C++ standard is having a type that represents pure "memory units" for storage, without associated arithmetic. So I think it makes sense to start adopting those in our codebase whenever char/uchar are used in places that don't actually treat them as numbers or as characters of a string. -
MarcoFalke commented at 8:22 AM on November 29, 2021: member
The long-term idea is to move from uint8_t to std::byte everywhere?
Yes, in places where raw bytes are handled. Though, this is mostly a style question, so there is no rush or need to switch existing code. For example, #23438 updates serialize to use Spans, and updating to
std::byteat the same time was "free". -
laanwj commented at 11:02 AM on November 29, 2021: member
Concept ACK
in the C++ standard is having a type that represents pure "memory units" for storage, without associated arithmetic.
Ok. I see. It would be incredibly difficult to port bitcoin to a system that doesn't have 8-bit memory units.
uint8_tjust makes sense to me in that regard. But no problem with making some code more general when it comes for free. -
sipa commented at 4:37 AM on November 30, 2021: member
@laanwj I don't think the point is generality; we very much assume that a byte is 8 bits all over the place. It's more a type safety thing:
std::byteindicates we're just talking about bytes, and not about numbers. They don't automatically get converted to ints or bools, for example. - DrahtBot added the label Needs rebase on Dec 3, 2021
- MarcoFalke force-pushed on Dec 3, 2021
- DrahtBot removed the label Needs rebase on Dec 3, 2021
-
laanwj commented at 1:26 PM on December 8, 2021: member
It's more a type safety thing: std::byte indicates we're just talking about bytes, and not about numbers
Thanks for the explanation. Yes a "memory area without interpretation" abstraction makes sense. I still have a hard time grasping when to use it though. We might want to add something to
developer-notes.mdfor guidance. (e.g. if it requires explicit casts everywhere, we don't want to use it in cases where the memory is in fact interpreted and treated as numbers, it would just add more boilerplate) -
MarcoFalke commented at 1:42 PM on December 8, 2021: member
I think a good rule of thumb would be to use it only for "raw memory" or "serialized memory". That is, any kind of memory that needs to be passed through a deserializer/parser before being useful.
For example: #23438 changes the serialize framework.
- DrahtBot added the label Needs rebase on Dec 15, 2021
- MarcoFalke force-pushed on Dec 15, 2021
- DrahtBot removed the label Needs rebase on Dec 15, 2021
- laanwj added this to the "Blockers" column in a project
- MarcoFalke marked this as a draft on Apr 17, 2022
- DrahtBot added the label Needs rebase on Apr 27, 2022
- MarcoFalke marked this as ready for review on Apr 27, 2022
-
fabdf81983
test: Add test for embedded null in hex string
Also, fix style in the corresponding function. The style change can be reviewed with "--word-diff-regex=."
- MarcoFalke force-pushed on Apr 27, 2022
-
util: Add ParseHex<std::byte>() helper fae1006019
-
refactor: Use Span of std::byte in CExtKey::SetSeed facd1fb911
- MarcoFalke force-pushed on Apr 27, 2022
- DrahtBot removed the label Needs rebase on Apr 27, 2022
-
pk-b2 commented at 6:04 PM on April 29, 2022: none
ACK https://github.com/bitcoin/bitcoin/pull/23595/commits/facd1fb911abfc595a3484ee53397eff515d4c40
Does it make sense to mention this specifically in the Developer notes when to use std::byte? https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures
-
MarcoFalke commented at 6:23 PM on April 29, 2022: member
Yeah, I am happy to review a pull request that changes the dev notes, though it seems unrelated to this change here, since we already use
std::byteextensively in existing code, so likely I won't be changing the docs in this pull. -
laanwj commented at 8:47 AM on May 20, 2022: member
Code review ACK facd1fb911abfc595a3484ee53397eff515d4c40
- laanwj merged this on May 20, 2022
- laanwj closed this on May 20, 2022
- laanwj removed this from the "Blockers" column in a project
-
w0xlt commented at 3:03 AM on May 21, 2022: contributor
- MarcoFalke deleted the branch on May 27, 2022
- sidhujag referenced this in commit 1eaf8344bc on May 28, 2022
- DrahtBot locked this on May 27, 2023