refactor: Make HexStr take a span #19660

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2020_06_hexstr_span changing 16 files +50 −70
  1. laanwj commented at 4:53 pm on August 4, 2020: member
    Make HexStr take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
  2. laanwj added the label Refactoring on Aug 4, 2020
  3. laanwj added the label Utils/log/libs on Aug 4, 2020
  4. laanwj force-pushed on Aug 4, 2020
  5. laanwj force-pushed on Aug 4, 2020
  6. DrahtBot commented at 5:29 pm on August 4, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
    • #16546 (External signer support - Wallet Box edition by Sjors)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  7. fanquake requested review from sipa on Aug 4, 2020
  8. in src/uint256.cpp:23 in c0bf0d79a8 outdated 
    18@@ -19,7 +19,11 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
19 template <unsigned int BITS>
20 std::string base_blob<BITS>::GetHex() const
21 {
22-    return HexStr(std::reverse_iterator<const uint8_t*>(m_data + sizeof(m_data)), std::reverse_iterator<const uint8_t*>(m_data));
23+    uint8_t m_data_rev[WIDTH];
24+    for (int i = 0; i < WIDTH; i++) {
    jonatack commented at 6:53 am on August 5, 2020:
    nit: Perhaps use the pre-increment (++i) operator.
    laanwj commented at 9:42 am on August 5, 2020:
    Sounds good to me
  9. jonatack commented at 6:53 am on August 5, 2020: member
    ACK c0bf0d79
  10. in src/rpc/request.cpp:81 in c0bf0d79a8 outdated 
    77@@ -78,7 +78,7 @@ bool GenerateAuthCookie(std::string *cookie_out)
78     const size_t COOKIE_SIZE = 32;
79     unsigned char rand_pwd[COOKIE_SIZE];
80     GetRandBytes(rand_pwd, COOKIE_SIZE);
81-    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd, rand_pwd+COOKIE_SIZE);
82+    std::string cookie = COOKIEAUTH_USER + ":" + HexStr(rand_pwd);
    promag commented at 9:36 am on August 5, 2020:
    Nice.
  11. in src/uint256.cpp:22 in c0bf0d79a8 outdated 
    18@@ -19,7 +19,11 @@ base_blob<BITS>::base_blob(const std::vector<unsigned char>& vch)
19 template <unsigned int BITS>
20 std::string base_blob<BITS>::GetHex() const
21 {
22-    return HexStr(std::reverse_iterator<const uint8_t*>(m_data + sizeof(m_data)), std::reverse_iterator<const uint8_t*>(m_data));
23+    uint8_t m_data_rev[WIDTH];
    promag commented at 9:40 am on August 5, 2020:
    Correct me if I’m wrong, but isn’t this duplicating memory usage?
    laanwj commented at 10:41 am on August 5, 2020:
    Not really: is is a local variable, all this does is create a small (32 bytes for uint256) temporary buffer on the stack. (I’ve opted to do this instead of creating a vector from the iterators, which would be a heap allocation)
  12. hebasto commented at 6:27 am on August 6, 2020: member
    Concept ACK.
  13. in src/util/strencodings.h:127 in c0bf0d79a8 outdated 
    143-}
144+/**
145+ * Convert a span of bytes to a lower-case hexadecimal string.
146+ */
147+std::string HexStr(const Span<const uint8_t> s);
148+static inline std::string HexStr(const Span<const char> s) { return HexStr(MakeUCharSpan(s)); }
    hebasto commented at 6:46 am on August 6, 2020:
    Does static is really needed here?
    laanwj commented at 9:22 am on August 6, 2020:
    I don’t think it is
  14. in src/util/strencodings.cpp:576 in c0bf0d79a8 outdated 
    568@@ -569,3 +569,16 @@ std::string Capitalize(std::string str)
569     str[0] = ToUpper(str.front());
570     return str;
571 }
572+
573+std::string HexStr(const Span<const uint8_t> s)
574+{
575+    std::string rv;
576+    static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
    hebasto commented at 6:47 am on August 6, 2020:

    nit:

    0    static constexpr char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
    laanwj commented at 9:23 am on August 6, 2020:

    I’m not sure. Does this make a difference for an array?

    Edit: well it works so I don’t think there any hurt in changing it…

  15. hebasto approved
  16. hebasto commented at 6:47 am on August 6, 2020: member
    ACK c0bf0d79a8ce532ea1cf0a656acba2db2d8f3112, tested on Linux Mint 20 (x86_64).
  17. promag commented at 8:39 am on August 6, 2020: member
    Code review ACK c0bf0d79a8ce532ea1cf0a656acba2db2d8f3112.
  18. hebasto approved
  19. hebasto commented at 9:28 am on August 6, 2020: member
    re-ACK c0c3262274a0767643b0e283aa5a5dcd30015f9b
  20. theStack commented at 4:53 pm on August 6, 2020: member
    Concept ACK
  21. refactor: Make HexStr take a span 
    Make HexStr take a span of bytes, instead of an awkward pair of
templated iterators.
    0a8aa626dd
  22. laanwj force-pushed on Aug 6, 2020
  23. laanwj commented at 5:42 pm on August 6, 2020: member
    Squashed the fixme commits
  24. hebasto approved
  25. hebasto commented at 6:05 pm on August 6, 2020: member
    re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3
  26. in src/rpc/util.cpp:263 in 0a8aa626dd 
    259@@ -260,7 +260,7 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
260         UniValue obj(UniValue::VOBJ);
261         obj.pushKV("iswitness", true);
262         obj.pushKV("witness_version", (int)id.version);
263-        obj.pushKV("witness_program", HexStr(id.program, id.program + id.length));
264+        obj.pushKV("witness_program", HexStr(Span<const unsigned char>(id.program, id.length)));
    laanwj commented at 7:39 am on August 7, 2020:
    Maybe we want to give WitnessUnknown .data() and .size() fields to avoid having to do this explicitly. Don’t know. Probably not in thie PR though.
  27. jonatack commented at 10:40 am on August 8, 2020: member
    re-ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3
  28. elichai commented at 11:20 am on August 8, 2020: contributor
    Nice! Code review ACK 0a8aa626dd69a357e1b798b07b64cf4177a464a3
  29. laanwj merged this on Aug 9, 2020
  30. laanwj closed this on Aug 9, 2020
  31. sidhujag referenced this in commit 0d802185f7 on Aug 9, 2020
  32. laanwj referenced this in commit 44ddcd887d on Aug 19, 2020
  33. sidhujag referenced this in commit d2b5dc4fbc on Aug 19, 2020
  34. Fabcien referenced this in commit 3288ce1b22 on Feb 9, 2021
  35. kittywhiskers referenced this in commit 478435dbd8 on May 18, 2021
  36. kittywhiskers referenced this in commit 5a4e75cdc3 on May 20, 2021
  37. kittywhiskers referenced this in commit dfd2a197d7 on May 20, 2021
  38. kittywhiskers referenced this in commit bc2f11f230 on May 20, 2021
  39. PastaPastaPasta referenced this in commit b76e7fec1f on May 21, 2021
  40. random-zebra referenced this in commit 33c2ae83aa on Aug 16, 2021
  41. christiancfifi referenced this in commit 452a33b410 on Aug 25, 2021
  42. christiancfifi referenced this in commit a00dd8142d on Aug 25, 2021
  43. christiancfifi referenced this in commit 32eeeec611 on Aug 25, 2021
  44. christiancfifi referenced this in commit 9d942d72f3 on Aug 25, 2021
  45. christiancfifi referenced this in commit a301e0a629 on Aug 25, 2021
  46. vijaydasmp referenced this in commit f451701388 on Sep 28, 2021
  47. DrahtBot locked this on Feb 15, 2022


laanwj DrahtBot jonatack promag hebasto theStack elichai


sipa

Labels
Refactoring Utils/log/libs

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: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me