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: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. DrahtBot cross-referenced this on Nov 26, 2021 from issue Replace MakeSpan helper with Span deduction guide by sipa
  5. 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?

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

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

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

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

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

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

  15. DrahtBot cross-referenced this on Dec 13, 2021 from issue refactor: Fix implicit integer sign changes in strencodings by MarcoFalke
  16. DrahtBot added the label Needs rebase on Dec 15, 2021
  17. MarcoFalke force-pushed on Dec 15, 2021
  18. DrahtBot removed the label Needs rebase on Dec 15, 2021
  19. DrahtBot cross-referenced this on Dec 18, 2021 from issue docs: avoid C-style casts; use modern C++ casts by PastaPastaPasta
  20. laanwj added this to the "Blockers" column in a project

  21. MarcoFalke cross-referenced this on Apr 4, 2022 from issue refactor: Remove ParseHex(const char*) from public interface by MarcoFalke
  22. DrahtBot cross-referenced this on Apr 4, 2022 from issue Modernize util/strencodings and util/string: `string_view` and `optional` by sipa
  23. MarcoFalke marked this as a draft on Apr 17, 2022
  24. DrahtBot added the label Needs rebase on Apr 27, 2022
  25. MarcoFalke marked this as ready for review on Apr 27, 2022
  26. 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
  27. MarcoFalke force-pushed on Apr 27, 2022
  28. util: Add ParseHex<std::byte>() helper fae1006019
  29. refactor: Use Span of std::byte in CExtKey::SetSeed facd1fb911
  30. MarcoFalke force-pushed on Apr 27, 2022
  31. DrahtBot removed the label Needs rebase on Apr 27, 2022
  32. pk-b2 commented at 6:04 PM on April 29, 2022: none
  33. 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.

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

    Code review ACK facd1fb911abfc595a3484ee53397eff515d4c40

  35. laanwj merged this on May 20, 2022
  36. laanwj closed this on May 20, 2022

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

  38. MarcoFalke deleted the branch on May 27, 2022
  39. sidhujag referenced this in commit 1eaf8344bc on May 28, 2022
  40. bitcoin 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-05-20 06:53 UTC