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

fanquake · fanquake · commit 6852d1d487a7 · 2024-09-05T13:46:22.000+01:00
faecca9 test: Use span for raw data (MarcoFalke) fac9736 test: Use string_view for json_tests (MarcoFalke) Pull request description: 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. ACKs for top commit: fjahr: re-ACK bitcoin/bitcoin@faecca9 TheCharlatan: ACK faecca9 stickies-v: ACK faecca9 hebasto: ACK faecca9, I have reviewed the code and the generated headers. Tree-SHA512: 1f4951c54aff11ba27c41fb70f2821bdb79e06ca0abae734b970bd0d64dda9d8cced824a891fd51b3e9d4e5715ee9eb49ed5d369010a45eca7c3bec9f8641235
diff --git a/cmake/module/GenerateHeaders.cmake b/cmake/module/GenerateHeaders.cmake
@@ -11,10 +11,10 @@ function(generate_header_from_json json_source_relpath)
   )
 endfunction()
 
-function(generate_header_from_raw raw_source_relpath)
+function(generate_header_from_raw raw_source_relpath raw_namespace)
   add_custom_command(
     OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h
-    COMMAND ${CMAKE_COMMAND} -DRAW_SOURCE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} -DHEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake
+    COMMAND ${CMAKE_COMMAND} -DRAW_SOURCE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} -DHEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h -DRAW_NAMESPACE=${raw_namespace} -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake
     DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake
     VERBATIM
   )
diff --git a/cmake/script/GenerateHeaderFromJson.cmake b/cmake/script/GenerateHeaderFromJson.cmake
@@ -5,10 +5,10 @@
 file(READ ${JSON_SOURCE_PATH} hex_content HEX)
 string(REGEX MATCHALL "([A-Za-z0-9][A-Za-z0-9])" bytes "${hex_content}")
 
-file(WRITE ${HEADER_PATH} "#include <string>\n")
+file(WRITE ${HEADER_PATH} "#include <string_view>\n")
 file(APPEND ${HEADER_PATH} "namespace json_tests{\n")
 get_filename_component(json_source_basename ${JSON_SOURCE_PATH} NAME_WE)
-file(APPEND ${HEADER_PATH} "static const std::string ${json_source_basename}{\n")
+file(APPEND ${HEADER_PATH} "inline constexpr char detail_${json_source_basename}_bytes[]{\n")
 
 set(i 0)
 foreach(byte ${bytes})
@@ -21,4 +21,6 @@ foreach(byte ${bytes})
   endif()
 endforeach()
 
-file(APPEND ${HEADER_PATH} "\n};};")
+file(APPEND ${HEADER_PATH} "\n};\n")
+file(APPEND ${HEADER_PATH} "inline constexpr std::string_view ${json_source_basename}{std::begin(detail_${json_source_basename}_bytes), std::end(detail_${json_source_basename}_bytes)};")
+file(APPEND ${HEADER_PATH} "\n}")
diff --git a/cmake/script/GenerateHeaderFromRaw.cmake b/cmake/script/GenerateHeaderFromRaw.cmake
@@ -5,18 +5,23 @@
 file(READ ${RAW_SOURCE_PATH} hex_content HEX)
 string(REGEX MATCHALL "([A-Za-z0-9][A-Za-z0-9])" bytes "${hex_content}")
 
+file(WRITE ${HEADER_PATH} "#include <cstddef>\n")
+file(APPEND ${HEADER_PATH} "#include <span>\n")
+file(APPEND ${HEADER_PATH} "namespace ${RAW_NAMESPACE} {\n")
 get_filename_component(raw_source_basename ${RAW_SOURCE_PATH} NAME_WE)
-file(WRITE ${HEADER_PATH} "static unsigned const char ${raw_source_basename}_raw[] = {\n")
+file(APPEND ${HEADER_PATH} "inline constexpr std::byte detail_${raw_source_basename}_raw[]{\n")
 
 set(i 0)
 foreach(byte ${bytes})
   math(EXPR i "${i} + 1")
   math(EXPR remainder "${i} % 8")
   if(remainder EQUAL 0)
-    file(APPEND ${HEADER_PATH} "0x${byte},\n")
+    file(APPEND ${HEADER_PATH} "std::byte{0x${byte}},\n")
   else()
-    file(APPEND ${HEADER_PATH} "0x${byte}, ")
+    file(APPEND ${HEADER_PATH} "std::byte{0x${byte}}, ")
   endif()
 endforeach()
 
-file(APPEND ${HEADER_PATH} "\n};")
+file(APPEND ${HEADER_PATH} "\n};\n")
+file(APPEND ${HEADER_PATH} "inline constexpr std::span ${raw_source_basename}{detail_${raw_source_basename}_raw};\n")
+file(APPEND ${HEADER_PATH} "}")
diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt
@@ -3,12 +3,11 @@
 # file COPYING or https://opensource.org/license/mit/.
 
 include(GenerateHeaders)
-generate_header_from_raw(data/block413567.raw)
+generate_header_from_raw(data/block413567.raw benchmark::data)
 
 add_executable(bench_bitcoin
   bench_bitcoin.cpp
   bench.cpp
-  data.cpp
   nanobench.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/data/block413567.raw.h
 # Benchmarks:
diff --git a/src/bench/checkblock.cpp b/src/bench/checkblock.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <bench/bench.h>
-#include <bench/data.h>
+#include <bench/data/block413567.raw.h>
 #include <chainparams.h>
 #include <common/args.h>
 #include <consensus/validation.h>
diff --git a/src/bench/data.cpp b/src/bench/data.cpp
diff --git a/src/bench/data.h b/src/bench/data.h
diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp
@@ -3,7 +3,7 @@
 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
 
 #include <bench/bench.h>
-#include <bench/data.h>
+#include <bench/data/block413567.raw.h>
 #include <chainparams.h>
 #include <flatfile.h>
 #include <node/blockstorage.h>
diff --git a/src/bench/readblock.cpp b/src/bench/readblock.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <bench/bench.h>
-#include <bench/data.h>
+#include <bench/data/block413567.raw.h>
 #include <flatfile.h>
 #include <node/blockstorage.h>
 #include <primitives/block.h>
diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <bench/bench.h>
-#include <bench/data.h>
+#include <bench/data/block413567.raw.h>
 #include <chain.h>
 #include <core_io.h>
 #include <primitives/block.h>
diff --git a/src/bench/strencodings.cpp b/src/bench/strencodings.cpp
@@ -3,7 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <bench/bench.h>
-#include <bench/data.h>
+#include <bench/data/block413567.raw.h>
 #include <span.h>
 #include <util/strencodings.h>
 
diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt
@@ -12,7 +12,7 @@ generate_header_from_json(data/script_tests.json)
 generate_header_from_json(data/sighash.json)
 generate_header_from_json(data/tx_invalid.json)
 generate_header_from_json(data/tx_valid.json)
-generate_header_from_raw(data/asmap.raw)
+generate_header_from_raw(data/asmap.raw test::data)
 
 # Do not use generator expressions in test sources because the
 # SOURCES property is processed to gather test suite macros.
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
@@ -47,11 +47,12 @@ static CService ResolveService(const std::string& ip, uint16_t port = 0)
 }
 
 
-static std::vector<bool> FromBytes(const unsigned char* source, int vector_size)
+static std::vector<bool> FromBytes(std::span<const std::byte> source)
 {
+    int vector_size(source.size() * 8);
     std::vector<bool> result(vector_size);
     for (int byte_i = 0; byte_i < vector_size / 8; ++byte_i) {
-        unsigned char cur_byte = source[byte_i];
+        uint8_t cur_byte{std::to_integer<uint8_t>(source[byte_i])};
         for (int bit_i = 0; bit_i < 8; ++bit_i) {
             result[byte_i * 8 + bit_i] = (cur_byte >> bit_i) & 1;
         }
@@ -576,7 +577,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy)
 // 101.8.0.0/16 AS8
 BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket)
 {
-    std::vector<bool> asmap = FromBytes(asmap_raw, sizeof(asmap_raw) * 8);
+    std::vector<bool> asmap = FromBytes(test::data::asmap);
     NetGroupManager ngm_asmap{asmap};
 
     CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE);
@@ -630,7 +631,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket)
 
 BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket)
 {
-    std::vector<bool> asmap = FromBytes(asmap_raw, sizeof(asmap_raw) * 8);
+    std::vector<bool> asmap = FromBytes(test::data::asmap);
     NetGroupManager ngm_asmap{asmap};
 
     CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE);
@@ -708,7 +709,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket)
 
 BOOST_AUTO_TEST_CASE(addrman_serialization)
 {
-    std::vector<bool> asmap1 = FromBytes(asmap_raw, sizeof(asmap_raw) * 8);
+    std::vector<bool> asmap1 = FromBytes(test::data::asmap);
     NetGroupManager netgroupman{asmap1};
 
     const auto ratio = GetCheckRatio(m_node);
diff --git a/src/test/util/json.cpp b/src/test/util/json.cpp
@@ -1,15 +1,15 @@
-// Copyright (c) 2023 The Bitcoin Core developers
+// Copyright (c) 2023-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <test/util/json.h>
 
-#include <string>
+#include <univalue.h>
 #include <util/check.h>
 
-#include <univalue.h>
+#include <string_view>
 
-UniValue read_json(const std::string& jsondata)
+UniValue read_json(std::string_view jsondata)
 {
     UniValue v;
     Assert(v.read(jsondata) && v.isArray());
diff --git a/src/test/util/json.h b/src/test/util/json.h
@@ -1,14 +1,14 @@
-// Copyright (c) 2023 The Bitcoin Core developers
+// Copyright (c) 2023-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #ifndef BITCOIN_TEST_UTIL_JSON_H
 #define BITCOIN_TEST_UTIL_JSON_H
 
-#include <string>
-
 #include <univalue.h>
 
-UniValue read_json(const std::string& jsondata);
+#include <string_view>
+
+UniValue read_json(std::string_view jsondata);
 
 #endif // BITCOIN_TEST_UTIL_JSON_H

Original file line number	Diff line number	Diff line change
`@@ -11,10 +11,10 @@ function(generate_header_from_json json_source_relpath)`
`11`	`11`	`)`
`12`	`12`	`endfunction()`
`13`	`13`
`14`		`-function(generate_header_from_raw raw_source_relpath)`
	`14`	`+function(generate_header_from_raw raw_source_relpath raw_namespace)`
`15`	`15`	`add_custom_command(`
`16`	`16`	`OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h`
`17`		`- COMMAND ${CMAKE_COMMAND} -DRAW_SOURCE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} -DHEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake`
	`17`	`+ COMMAND ${CMAKE_COMMAND} -DRAW_SOURCE_PATH=${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} -DHEADER_PATH=${CMAKE_CURRENT_BINARY_DIR}/${raw_source_relpath}.h -DRAW_NAMESPACE=${raw_namespace} -P ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake`
`18`	`18`	`DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/${raw_source_relpath} ${PROJECT_SOURCE_DIR}/cmake/script/GenerateHeaderFromRaw.cmake`
`19`	`19`	`VERBATIM`
`20`	`20`	`)`