test: Use std::span and std::string

maflcko commented at 2:14 pm on September 3, 2024: member

The build system converts raw data into a C++ header file for tests.

This change modernizes the code to use the convenience wrappers std::span and std::string_view, so that redundant copies can be avoided.

test: Use string_view for json_tests

This avoids a static constructor of the global std::string, and rules
out possibly expensive and implicit copies of the string completely.

fac973647d

DrahtBot commented at 2:14 pm on September 3, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, fjahr, stickies-v, hebasto
Concept ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Sep 3, 2024

hebasto commented at 2:19 pm on September 3, 2024: member

Concept ACK.

maflcko commented at 2:40 pm on September 3, 2024: member

On my system the test_bitcoin and bench_bitcoin binaries are smaller by 30kB, and 4kB respectively. But that isn’t the goal of this pull, just a nice side-effect.

fjahr commented at 2:45 pm on September 3, 2024: contributor

Concept ACK

BrandonOdiwuor commented at 6:49 pm on September 4, 2024: contributor

Concept ACK

in cmake/script/GenerateHeaderFromJson.cmake:11 in fac973647d outdated

 4@@ -5,10 +5,10 @@
 5 file(READ ${JSON_SOURCE_PATH} hex_content HEX)
 6 string(REGEX MATCHALL "([A-Za-z0-9][A-Za-z0-9])" bytes "${hex_content}")
 7 
 8-file(WRITE ${HEADER_PATH} "#include <string>\n")
 9+file(WRITE ${HEADER_PATH} "#include <string_view>\n")
10 file(APPEND ${HEADER_PATH} "namespace json_tests{\n")
11 get_filename_component(json_source_basename ${JSON_SOURCE_PATH} NAME_WE)
12-file(APPEND ${HEADER_PATH} "static const std::string ${json_source_basename}{\n")
13+file(APPEND ${HEADER_PATH} "inline constexpr char detail_${json_source_basename}_bytes[]{\n")

fjahr commented at 9:08 pm on September 4, 2024:

What does the detail_ prefix mean?

maflcko commented at 5:04 am on September 5, 2024:

What does the detail_ prefix mean?

It is just the lazy version of namespace detail, see https://duckduckgo.com/?q=namespace+detail or git grep "namespace detail" to denote internal details that should not be considered exposed.

Happy to wrap the symbol into a namespace detail properly.

fjahr commented at 7:40 am on September 5, 2024:

Ok, I hadn’t come across this yet. I don’t have a preference, you can leave it as is.

fjahr commented at 9:10 pm on September 4, 2024: contributor

Code review ACK fa0321cc742754af4e5822d4227d4136a6f4f467

DrahtBot requested review from BrandonOdiwuor on Sep 4, 2024

DrahtBot requested review from hebasto on Sep 4, 2024

in src/test/addrman_tests.cpp:55 in fa0321cc74 outdated

52 {
53+    int vector_size(source.size() * 8);
54     std::vector<bool> result(vector_size);
55     for (int byte_i = 0; byte_i < vector_size / 8; ++byte_i) {
56-        unsigned char cur_byte = source[byte_i];
57+        uint8_t cur_byte{uint8_t(source[byte_i])};

TheCharlatan commented at 8:07 am on September 5, 2024:

Would it better to use std::to_integer here? It does the same thing, but maybe it would be good to use it for its compile-time guarantees.

maflcko commented at 8:53 am on September 5, 2024:

Sure, added compile time check that ensures std::is_integral_v<uint8_t>.

Also, fixed typo in commit message.

Should be trivial to re-ACK

TheCharlatan commented at 8:22 am on September 5, 2024: contributor

Approach ACK

This looks good, just left a small question, and there is a typo at the start of the second commit’s message.

maflcko force-pushed on Sep 5, 2024

test: Use span for raw data

This change allows to drop brittle sizeof calls in favor of the
std::span::size method.

Other improvements include:

* Use of a namespace to mark test and bench data
* Use of the modern std::byte
* Drop of a no longer used std::vector copy and the bench/data module

faecca9a85

maflcko force-pushed on Sep 5, 2024

TheCharlatan approved

TheCharlatan commented at 9:04 am on September 5, 2024: contributor

ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9

DrahtBot requested review from fjahr on Sep 5, 2024

fjahr commented at 9:08 am on September 5, 2024: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/faecca9a85c1c1917d024f55cca34d19cc94f3b9

Only changes since last review were usage of std::to_integer and some shuffling of newline characters.

in src/test/addrman_tests.cpp:52 in faecca9a85

46@@ -47,11 +47,12 @@ static CService ResolveService(const std::string& ip, uint16_t port = 0)
47 }
48 
49 
50-static std::vector<bool> FromBytes(const unsigned char* source, int vector_size)
51+static std::vector<bool> FromBytes(std::span<const std::byte> source)
52 {
53+    int vector_size(source.size() * 8);

stickies-v commented at 11:42 am on September 5, 2024:

nit: I think the code is more clear without this helper variable, avoids the first multiplying and then dividing by 8 again:

 0diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
 1index da6ff77924..fd7a1f2ba2 100644
 2--- a/src/test/addrman_tests.cpp
 3+++ b/src/test/addrman_tests.cpp
 4@@ -49,11 +49,10 @@ static CService ResolveService(const std::string& ip, uint16_t port = 0)
 5 
 6 static std::vector<bool> FromBytes(std::span<const std::byte> source)
 7 {
 8-    int vector_size(source.size() * 8);
 9-    std::vector<bool> result(vector_size);
10-    for (int byte_i = 0; byte_i < vector_size / 8; ++byte_i) {
11+    std::vector<bool> result(source.size() * 8);
12+    for (size_t byte_i = 0; byte_i < source.size(); ++byte_i) {
13         uint8_t cur_byte{std::to_integer<uint8_t>(source[byte_i])};
14-        for (int bit_i = 0; bit_i < 8; ++bit_i) {
15+        for (size_t bit_i = 0; bit_i < 8; ++bit_i) {
16             result[byte_i * 8 + bit_i] = (cur_byte >> bit_i) & 1;
17         }
18     }

maflcko commented at 12:17 pm on September 5, 2024:

Will push if I have to re-touch.

stickies-v approved

stickies-v commented at 12:01 pm on September 5, 2024: contributor

ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9

Commit messages were very helpful, thank you.

hebasto approved

hebasto commented at 12:04 pm on September 5, 2024: member

ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9, I have reviewed the code and the generated headers.

fanquake merged this on Sep 5, 2024

fanquake closed this on Sep 5, 2024

maflcko deleted the branch on Sep 5, 2024

test: Use std::span and std::string_view for raw data #30796

Code Coverage

Reviews