Merge bitcoin/bitcoin#28168: refactor: Remove unused raw-pointer read helper from univalue

fanquake · fanquake · commit 8aa77a77e6fa · 2023-07-28T12:04:49.000+01:00
fa940f4 Remove unused raw-pointer read helper from univalue (MarcoFalke) Pull request description: The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`. Fix both issues by removing them. Also, simplify the tests code by removing a `std::string` constructor where possible. ACKs for top commit: stickies-v: utACK fa940f4 TheCharlatan: tACK fa940f4 Tree-SHA512: 60c154c1046f01551335af79bf820a6104844f63e89977271b4336b3cd06ac3bab1379e18b7bc61d12bef7446029e91c16541ddecf9e88bc8bc897fc1f6ee2c8
diff --git a/build_msvc/test_bitcoin/test_bitcoin.vcxproj b/build_msvc/test_bitcoin/test_bitcoin.vcxproj
@@ -68,7 +68,7 @@
       <RawTestFile Include="..\..\src\test\data\*.raw" />
     </ItemGroup>
     <HeaderFromHexdump RawFilePath="%(RawTestFile.FullPath)" HeaderFilePath="%(RawTestFile.FullPath).h" SourceHeader="static unsigned const char %(RawTestFile.Filename)_raw[] = {" SourceFooter="};" />
-    <HeaderFromHexdump RawFilePath="%(JsonTestFile.FullPath)" HeaderFilePath="%(JsonTestFile.FullPath).h" SourceHeader="namespace json_tests{ static unsigned const char %(JsonTestFile.Filename)[] = {" SourceFooter="};}" />
+    <HeaderFromHexdump RawFilePath="%(JsonTestFile.FullPath)" HeaderFilePath="%(JsonTestFile.FullPath).h" SourceHeader="#include &lt;string&gt;&#x0D;&#x0A;namespace json_tests{ static const std::string %(JsonTestFile.Filename){" SourceFooter="};}" />
   </Target>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
@@ -427,8 +427,9 @@ endif
 %.json.h: %.json
 	@$(MKDIR_P) $(@D)
 	$(AM_V_GEN) { \
+	 echo "#include <string>" && \
 	 echo "namespace json_tests{" && \
-	 echo "static unsigned const char $(*F)[] = {" && \
+	 echo "static const std::string $(*F){" && \
 	 $(HEXDUMP) -v -e '8/1 "0x%02x, "' -e '"\n"' $< | $(SED) -e 's/0x  ,//g' && \
 	 echo "};};"; \
 	} > "$@.new" && mv -f "$@.new" "$@"
diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp
@@ -23,7 +23,7 @@ BOOST_FIXTURE_TEST_SUITE(base58_tests, BasicTestingSetup)
 // Goal: test low-level base58 encoding functionality
 BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
 {
-    UniValue tests = read_json(std::string(json_tests::base58_encode_decode, json_tests::base58_encode_decode + sizeof(json_tests::base58_encode_decode)));
+    UniValue tests = read_json(json_tests::base58_encode_decode);
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
         std::string strTest = test.write();
@@ -43,7 +43,7 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
 // Goal: test low-level base58 decoding functionality
 BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
 {
-    UniValue tests = read_json(std::string(json_tests::base58_encode_decode, json_tests::base58_encode_decode + sizeof(json_tests::base58_encode_decode)));
+    UniValue tests = read_json(json_tests::base58_encode_decode);
     std::vector<unsigned char> result;
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
diff --git a/src/test/blockfilter_tests.cpp b/src/test/blockfilter_tests.cpp
@@ -128,9 +128,7 @@ BOOST_AUTO_TEST_CASE(blockfilter_basic_test)
 BOOST_AUTO_TEST_CASE(blockfilters_json_test)
 {
     UniValue json;
-    std::string json_data(json_tests::blockfilters,
-                          json_tests::blockfilters + sizeof(json_tests::blockfilters));
-    if (!json.read(json_data) || !json.isArray()) {
+    if (!json.read(json_tests::blockfilters) || !json.isArray()) {
         BOOST_ERROR("Parse error.");
         return;
     }
diff --git a/src/test/key_io_tests.cpp b/src/test/key_io_tests.cpp
@@ -22,7 +22,7 @@ BOOST_FIXTURE_TEST_SUITE(key_io_tests, BasicTestingSetup)
 // Goal: check that parsed keys match test payload
 BOOST_AUTO_TEST_CASE(key_io_valid_parse)
 {
-    UniValue tests = read_json(std::string(json_tests::key_io_valid, json_tests::key_io_valid + sizeof(json_tests::key_io_valid)));
+    UniValue tests = read_json(json_tests::key_io_valid);
     CKey privkey;
     CTxDestination destination;
     SelectParams(ChainType::MAIN);
@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(key_io_valid_parse)
 // Goal: check that generated keys match test vectors
 BOOST_AUTO_TEST_CASE(key_io_valid_gen)
 {
-    UniValue tests = read_json(std::string(json_tests::key_io_valid, json_tests::key_io_valid + sizeof(json_tests::key_io_valid)));
+    UniValue tests = read_json(json_tests::key_io_valid);
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
@@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(key_io_valid_gen)
 // Goal: check that base58 parsing code is robust against a variety of corrupted data
 BOOST_AUTO_TEST_CASE(key_io_invalid)
 {
-    UniValue tests = read_json(std::string(json_tests::key_io_invalid, json_tests::key_io_invalid + sizeof(json_tests::key_io_invalid))); // Negative testcases
+    UniValue tests = read_json(json_tests::key_io_invalid); // Negative testcases
     CKey privkey;
     CTxDestination destination;
 
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
@@ -20,7 +20,7 @@
 static UniValue JSON(std::string_view json)
 {
     UniValue value;
-    BOOST_CHECK(value.read(json.data(), json.size()));
+    BOOST_CHECK(value.read(json));
     return value;
 }
 
diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp
@@ -394,7 +394,7 @@ BOOST_AUTO_TEST_CASE(bip341_spk_test_vectors)
     using control_set = decltype(TaprootSpendData::scripts)::mapped_type;
 
     UniValue tests;
-    tests.read((const char*)json_tests::bip341_wallet_vectors, sizeof(json_tests::bip341_wallet_vectors));
+    tests.read(json_tests::bip341_wallet_vectors);
 
     const auto& vectors = tests["scriptPubKey"];
 
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
@@ -890,7 +890,7 @@ BOOST_AUTO_TEST_CASE(script_build)
     std::set<std::string> tests_set;
 
     {
-        UniValue json_tests = read_json(std::string(json_tests::script_tests, json_tests::script_tests + sizeof(json_tests::script_tests)));
+        UniValue json_tests = read_json(json_tests::script_tests);
 
         for (unsigned int idx = 0; idx < json_tests.size(); idx++) {
             const UniValue& tv = json_tests[idx];
@@ -929,7 +929,7 @@ BOOST_AUTO_TEST_CASE(script_json_test)
     // scripts.
     // If a witness is given, then the last value in the array should be the
     // amount (nValue) to use in the crediting tx
-    UniValue tests = read_json(std::string(json_tests::script_tests, json_tests::script_tests + sizeof(json_tests::script_tests)));
+    UniValue tests = read_json(json_tests::script_tests);
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
@@ -1743,7 +1743,7 @@ BOOST_AUTO_TEST_CASE(script_assets_test)
 BOOST_AUTO_TEST_CASE(bip341_keypath_test_vectors)
 {
     UniValue tests;
-    tests.read((const char*)json_tests::bip341_wallet_vectors, sizeof(json_tests::bip341_wallet_vectors));
+    tests.read(json_tests::bip341_wallet_vectors);
 
     const auto& vectors = tests["keyPathSpending"];
 
diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp
@@ -162,7 +162,7 @@ BOOST_AUTO_TEST_CASE(sighash_test)
 // Goal: check that SignatureHash generates correct hash
 BOOST_AUTO_TEST_CASE(sighash_from_data)
 {
-    UniValue tests = read_json(std::string(json_tests::sighash, json_tests::sighash + sizeof(json_tests::sighash)));
+    UniValue tests = read_json(json_tests::sighash);
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
@@ -190,7 +190,7 @@ BOOST_AUTO_TEST_CASE(tx_valid)
 {
     BOOST_CHECK_MESSAGE(CheckMapFlagNames(), "mapFlagNames is missing a script verification flag");
     // Read tests from test/data/tx_valid.json
-    UniValue tests = read_json(std::string(json_tests::tx_valid, json_tests::tx_valid + sizeof(json_tests::tx_valid)));
+    UniValue tests = read_json(json_tests::tx_valid);
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
@@ -278,7 +278,7 @@ BOOST_AUTO_TEST_CASE(tx_valid)
 BOOST_AUTO_TEST_CASE(tx_invalid)
 {
     // Read tests from test/data/tx_invalid.json
-    UniValue tests = read_json(std::string(json_tests::tx_invalid, json_tests::tx_invalid + sizeof(json_tests::tx_invalid)));
+    UniValue tests = read_json(json_tests::tx_invalid);
 
     for (unsigned int idx = 0; idx < tests.size(); idx++) {
         const UniValue& test = tests[idx];
diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h
@@ -7,13 +7,15 @@
 #define BITCOIN_UNIVALUE_INCLUDE_UNIVALUE_H
 
 #include <charconv>
+#include <cstddef>
 #include <cstdint>
-#include <cstring>
 #include <map>
 #include <stdexcept>
 #include <string>
 #include <string_view>
+#include <system_error>
 #include <type_traits>
+#include <utility>
 #include <vector>
 
 class UniValue {
@@ -94,9 +96,7 @@ class UniValue {
     std::string write(unsigned int prettyIndent = 0,
                       unsigned int indentLevel = 0) const;
 
-    bool read(const char *raw, size_t len);
-    bool read(const char *raw) { return read(raw, strlen(raw)); }
-    bool read(std::string_view raw) { return read(raw.data(), raw.size()); }
+    bool read(std::string_view raw);
 
 private:
     UniValue::VType typ;
diff --git a/src/univalue/lib/univalue_read.cpp b/src/univalue/lib/univalue_read.cpp
@@ -5,10 +5,11 @@
 #include <univalue.h>
 #include <univalue_utffilter.h>
 
-#include <cstdio>
 #include <cstdint>
+#include <cstdio>
 #include <cstring>
 #include <string>
+#include <string_view>
 #include <vector>
 
 /*
@@ -259,7 +260,7 @@ enum expect_bits : unsigned {
 #define setExpect(bit) (expectMask |= EXP_##bit)
 #define clearExpect(bit) (expectMask &= ~EXP_##bit)
 
-bool UniValue::read(const char *raw, size_t size)
+bool UniValue::read(std::string_view str_in)
 {
     clear();
 
@@ -270,7 +271,8 @@ bool UniValue::read(const char *raw, size_t size)
     unsigned int consumed;
     enum jtokentype tok = JTOK_NONE;
     enum jtokentype last_tok = JTOK_NONE;
-    const char* end = raw + size;
+    const char* raw{str_in.data()};
+    const char* end{raw + str_in.size()};
     do {
         last_tok = tok;
 
diff --git a/src/univalue/test/unitester.cpp b/src/univalue/test/unitester.cpp
@@ -153,7 +153,7 @@ void no_nul_test()
 {
     char buf[] = "___[1,2,3]___";
     UniValue val;
-    assert(val.read(buf + 3, 7));
+    assert(val.read({buf + 3, 7}));
 }
 
 int main (int argc, char *argv[])

Original file line number	Diff line number	Diff line change
`@@ -128,9 +128,7 @@ BOOST_AUTO_TEST_CASE(blockfilter_basic_test)`
`128`	`128`	`BOOST_AUTO_TEST_CASE(blockfilters_json_test)`
`129`	`129`	`{`
`130`	`130`	`UniValue json;`
`131`		`- std::string json_data(json_tests::blockfilters,`
`132`		`- json_tests::blockfilters + sizeof(json_tests::blockfilters));`
`133`		`- if (!json.read(json_data) \|\| !json.isArray()) {`
	`131`	`+ if (!json.read(json_tests::blockfilters) \|\| !json.isArray()) {`
`134`	`132`	`BOOST_ERROR("Parse error.");`
`135`	`133`	`return;`
`136`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@`
`20`	`20`	`static UniValue JSON(std::string_view json)`
`21`	`21`	`{`
`22`	`22`	`UniValue value;`
`23`		`- BOOST_CHECK(value.read(json.data(), json.size()));`
	`23`	`+ BOOST_CHECK(value.read(json));`
`24`	`24`	`return value;`
`25`	`25`	`}`
`26`	`26`
Original file line number	Diff line number	Diff line change
`@@ -162,7 +162,7 @@ BOOST_AUTO_TEST_CASE(sighash_test)`
`162`	`162`	`// Goal: check that SignatureHash generates correct hash`
`163`	`163`	`BOOST_AUTO_TEST_CASE(sighash_from_data)`
`164`	`164`	`{`
`165`		`- UniValue tests = read_json(std::string(json_tests::sighash, json_tests::sighash + sizeof(json_tests::sighash)));`
	`165`	`+ UniValue tests = read_json(json_tests::sighash);`
`166`	`166`
`167`	`167`	`for (unsigned int idx = 0; idx < tests.size(); idx++) {`
`168`	`168`	`const UniValue& test = tests[idx];`