dead code: Remove dead option in HexStr conversion #15573

pull ldm5180 wants to merge 1 commits into bitcoin:master from ldm5180:hexstr_dead_code changing 2 files +5 −47

ldm5180 commented at 5:56 PM on March 10, 2019: contributor
Problem:
- Nothing uses the fspaces argument to HexStr() besides unit tests. This argument results in extra complexity and a small performance decrease within the function.
Solution:
- Remove unused fspaces option.
- Remove associated unit tests.
fanquake added the label Refactoring on Mar 10, 2019
Empact commented at 10:38 PM on March 10, 2019: member

I wouldn’t call the performance difference worthwhile, but I’m all for removing dead code.

promag commented at 10:56 PM on March 10, 2019: member

Concept ACK, if it's not used then it can be removed.

I don't think the performance gain justifies this change. Anyway, I think the following is enough:

--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -121,17 +121,15 @@ NODISCARD bool ParseUInt64(const std::string& str, uint64_t *out);
 NODISCARD bool ParseDouble(const std::string& str, double *out);

 template<typename T>
-std::string HexStr(const T itbegin, const T itend, bool fSpaces=false)
+std::string HexStr(const T itbegin, const T itend)
 {
     std::string rv;
     static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
                                      '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
-    rv.reserve((itend-itbegin)*3);
+    rv.reserve((itend-itbegin) * 2);
     for(T it = itbegin; it < itend; ++it)
     {
         unsigned char val = (unsigned char)(*it);
-        if(fSpaces && it != itbegin)
-            rv.push_back(' ');
         rv.push_back(hexmap[val>>4]);
         rv.push_back(hexmap[val&15]);
     }
@@ -140,9 +138,9 @@ std::string HexStr(const T itbegin, const T itend, bool fSpaces=false)
 }

 template<typename T>
-inline std::string HexStr(const T& vch, bool fSpaces=false)
+inline std::string HexStr(const T& vch)
 {
-    return HexStr(vch.begin(), vch.end(), fSpaces);
+    return HexStr(vch.begin(), vch.end());
 }

 /**

sipa commented at 11:00 PM on March 10, 2019: member

HexStr does not in any way affect the Base58 benchmarks, I think?
ldm5180 force-pushed on Mar 11, 2019
ldm5180 commented at 12:31 AM on March 11, 2019: contributor

Limited change to only removing dead code and updated PR details.
in src/util/strencodings.h:130 in 24a9efed02 outdated
```
 126 |  {
 127 |      std::string rv;
 128 |      static const char hexmap[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
 129 |                                       '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
 130 | -    rv.reserve((itend-itbegin)*3);
 131 | +    rv.reserve(std::distance(itbegin, itend) * 2);
```
Empact commented at 1:46 AM on March 11, 2019:

#include <iterator>?

ldm5180 commented at 2:21 AM on March 11, 2019:

Thanks for noticing. Done.

Empact commented at 2:44 AM on March 11, 2019:

Why in uint256.h?

ldm5180 commented at 3:46 AM on March 11, 2019:

I have no idea. Mix-up on my part. Fixed now.
DrahtBot commented at 1:47 AM on March 11, 2019: 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:
- #15576 (constexpr: Increase constexpr usage in strencodings by ldm5180)
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.
ldm5180 force-pushed on Mar 11, 2019

dead code: Remove dead option in HexStr conversion

Problem:
- Nothing uses the `fspaces` argument to `HexStr()` besides unit
  tests. This argument results in extra complexity and a small
  performance decrease within the function for branch evalulation.

Solution:
- Remove unused `fspaces` option.

56f1d28d9b

ldm5180 force-pushed on Mar 11, 2019
Empact commented at 3:57 AM on March 11, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15573/commits/56f1d28d9b606397c0c746b57243a0f2b971ff8a
practicalswift commented at 10:18 AM on March 11, 2019: contributor

utACK 56f1d28d9b606397c0c746b57243a0f2b971ff8a

-42 lines: very nice! Thanks for removing this cruft.
MarcoFalke commented at 2:24 PM on March 11, 2019: member

utACK 56f1d28
promag commented at 2:32 PM on March 11, 2019: member

utACK 56f1d28.

HexStr does not in any way affect the Base58 benchmarks, I think? @ldm5180 why did you use those benchs in the first place?
laanwj merged this on Mar 12, 2019
laanwj closed this on Mar 12, 2019
laanwj referenced this in commit c3b1cb958f on Mar 12, 2019
jasonbcox referenced this in commit b22266442d on Oct 3, 2020
random-zebra referenced this in commit 33c2ae83aa on Aug 16, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

dead code: Remove dead option in HexStr conversion #15573

Conflicts