8000 refactor: Replace HexStr(o.begin(), o.end()) with HexStr(o) by laanwj · Pull Request #19373 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: Replace HexStr(o.begin(), o.end()) with HexStr(o) #19373

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

laanwj
Copy link
Member
@laanwj laanwj commented Jun 24, 2020

HexStr can be called with anything that bas begin() and end() functions, so clean up the redundant calls.

(context: I tried to convert HexStr to use span, but this turns out to be somewhat more involved than I thought, because of the limitation to pre-c++17 Span lacking iterator-based constructor) . This commit is a first step which stands on its own though)

Copy link
Member
@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup.

almost-ACK 282df90 modulo when building the fuzzers

test/fuzz/decode_tx.cpp:17:39: error: no matching constructor for initialization of 'std::string' (aka 'basic_string<char>')
    const std::string tx_hex = HexStr(std::string{buffer});
                                      ^          ~~~~~~~~

HexStr can be called with anything that bas `begin()` and `end()` functions,
so clean up the redundant calls.
@laanwj
Copy link
Member Author
laanwj commented Jun 24, 2020

Thanks!
Re-pushed, hopefully fixing the fuzzer issue.

@troygiorshev
Copy link
Contributor
troygiorshev commented Jun 24, 2020

ACK bd93e32

When running all of the fuzz tests, descriptor_parse and parse_univalue break for me. However, those aren't touched by this PR so I'm assuming it's a problem with my machine. When those are excluded, everything works fine.

Looking forward to eventually being able to build the fuzz tests at the same time as everything else :)

@jonatack
Copy link
Member

@troygiorshev maybe make clean or make distclean might help: ./autogen.sh ; ./configure --enable-c++17 --enable-fuzz --with-sanitizers=address,fuzzer,undefined CC=clang CXX=clang++ && make clean && make -j <nproc>

@maflcko
Copy link
Member
maflcko commented Jun 24, 2020

I ran sed -i --regexp-extended -e 's/HexStr\(([^(]+)\.begin\(\), *([^(]+)\.end\(\)\)/HexStr(\1)/g' $(git grep -l HexStr) and arrived at the same result.

review ACK bd93e32 🔌

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK bd93e32292c96b671e71223032ff8f660ce27c5d 🔌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgRIAv/Uk2ihLCqBuVZ0KblTbUu+g5OGfsCXD9KS2QFTVWWMoa7uNFfJW1AQfVq
xwu1u7fHAxE51Pd8pshRLD7PjNRT1vab5wW5NXp0UuBJMjUb8uoQ1x3CYLHN24wV
c434W65R92ZLMkYUM+gBixh3DkiRXd89mnihcxqp15Ls7yhZJIW08e5DZKIKNbiH
HfM+PFkdq4DJV473hhASjSKgdVBngZuuOdzk6Vi53jOQFh7IE38YeT71xBJ0IRBh
qiCfcXsS/mzRrFe7PHMG9GPlWzMP/ede5ICOrauncjgA/3UBR6i1o9RfzX6b885o
M9c5dHdkhd3WjuwEBYrCj9gtbMRyqC7kZbLEYhWcfR/Psp/0SKyF4FAP8O8AItiC
rc4BQYJn/3GG7b7SUReu2PyyDFPwJW3r68x6WQEbSclHq+3iRfNKTiOBe1vc0zI+
r2FnahlKrw3/hqz4jDUJfgooeiKHrev2SJOKZJOhzGqyL4ujnTdKp+NRpoRRXy+t
hxQ3BPag
=NgtJ
-----END PGP SIGNATURE-----

Timestamp of file with hash c29a816f655c66172f117b610389a5a791097d2c042b69406155146f2572f5c9 -

@maflcko maflcko merged commit 532b134 into bitcoin:master Jun 24, 2020
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 1, 2021
Summary:
```
HexStr can be called with anything that bas begin() and end() functions,
so clean up the redundant calls.

(context: I tried to convert HexStr to use span, but this turns out to
be somewhat more involved than I thought, because of the limitation to
pre-c++17 Span lacking iterator-based constructor) . This commit is a
first step which stands on its own though)
```

Backport of core [[bitcoin/bitcoin#19373 | PR19373]].

Test Plan:
  ninja all check-all

  ninja bitcoin-fuzzers
  ./test/fuzz/test_runner.py <path_to_corpus>

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9122
kwvg added a commit to kwvg/dash that referenced this pull request Mar 8, 2021
Using GNU sed `sed -i --regexp-extended -e 's/HexStr\(([^(]+)\.begin\(\), *([^(]+)\.end\(\)\)/HexStr(\1)/g' $(git grep -l HexStr)`

Taken from comment bitcoin#19373 (comment) on bitcoin#19373
kwvg added a commit to kwvg/dash that referenced this pull request May 18, 2021
kwvg added a commit to kwvg/dash that referenced this pull request May 19, 2021
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0