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
  1. MarcoFalke commented at 4:05 PM on November 25, 2021: member

    This adds the hex->std::byte helper after the std::byte->hex helper was added in commit 9394964f6b9d1cf1220a4eca17ba18dc49ae876d

  2. MarcoFalke added the label Refactoring on Nov 25, 2021
  3. 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.

  4. laanwj commented at 3:21 PM on November 28, 2021: member

    The long-term idea is to move from uint8_t to std::byte everywhere?

  5. sipa commented at 4:06 PM on November 28, 2021: member

    I believe that the rationale behind std::byte in 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.

  6. 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::byte at the same time was "free".

  7. 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_t just makes sense to me in that regard. But no problem with making some code more general when it comes for free.

  8. 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::byte indicates we're just talking about bytes, and not about numbers. They don't automatically get converted to ints or bools, for example.

  9. DrahtBot added the label Needs rebase on Dec 3, 2021
  10. MarcoFalke force-pushed on Dec 3, 2021
  11. DrahtBot removed the label Needs rebase on Dec 3, 2021
  12. 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.md for 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)

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

  14. DrahtBot added the label Needs rebase on Dec 15, 2021
  15. MarcoFalke force-pushed on Dec 15, 2021
  16. DrahtBot removed the label Needs rebase on Dec 15, 2021
  17. laanwj added this to the "Blockers" column in a project

  18. MarcoFalke marked this as a draft on Apr 17, 2022
  19. DrahtBot added the label Needs rebase on Apr 27, 2022
  20. MarcoFalke marked this as ready for review on Apr 27, 2022
  21. 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=."
    fabdf81983
  22. MarcoFalke force-pushed on Apr 27, 2022
  23. util: Add ParseHex<std::byte>() helper fae1006019
  24. refactor: Use Span of std::byte in CExtKey::SetSeed facd1fb911
  25. MarcoFalke force-pushed on Apr 27, 2022
  26. DrahtBot removed the label Needs rebase on Apr 27, 2022
  27. pk-b2 commented at 6:04 PM on April 29, 2022: none
  28. 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::byte extensively in existing code, so likely I won't be changing the docs in this pull.

  29. laanwj commented at 8:47 AM on May 20, 2022: member

    Code review ACK facd1fb911abfc595a3484ee53397eff515d4c40

  30. laanwj merged this on May 20, 2022
  31. laanwj closed this on May 20, 2022

  32. laanwj removed this from the "Blockers" column in a project

  33. MarcoFalke deleted the branch on May 27, 2022
  34. sidhujag referenced this in commit 1eaf8344bc on May 28, 2022
  35. DrahtBot locked this on May 27, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me