diff options
author | fanquake <fanquake@gmail.com> | 2023-03-03 15:23:10 +0100 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2023-03-03 15:23:43 +0100 |
commit | 3b88c8502534f0dc94e0abcb04ffa80ba8bd7f01 (patch) | |
tree | ef9762eee94274f414ee8c5205a674cab7576580 /src/rpc/client.cpp | |
parent | a12b27a2a6dafe12fbf59815b88f2404617625d6 (diff) | |
parent | 545ff924ab6303ffabd91fdfc4f0a4962daf133c (diff) | |
download | bitcoin-3b88c8502534f0dc94e0abcb04ffa80ba8bd7f01.tar.xz |
Merge bitcoin/bitcoin#26612: refactor: RPC: pass named argument value as string_view
545ff924ab6303ffabd91fdfc4f0a4962daf133c refactor: use string_view for RPC named argument values (stickies-v)
7727603e44f8f674e0fc8389e78047e2b56e6052 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e599012721549d4c20b1b37fcc5ee7b961b6 test: add cases to JSON parsing (stickies-v)
Pull request description:
Inspired by MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26506#discussion_r1036149426). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change.
## Questions
- ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~
- Cleared up by https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`.
ACKs for top commit:
ryanofsky:
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
MarcoFalke:
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
Diffstat (limited to 'src/rpc/client.cpp')
-rw-r--r-- | src/rpc/client.cpp | 31 |
1 files changed, 16 insertions, 15 deletions
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index eb91a151b5..9449b9d197 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -4,10 +4,13 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <rpc/client.h> +#include <tinyformat.h> #include <util/system.h> #include <set> #include <stdint.h> +#include <string> +#include <string_view> class CRPCConvertParam { @@ -229,15 +232,15 @@ public: CRPCConvertTable(); /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ - UniValue ArgToUniValue(const std::string& arg_value, const std::string& method, int param_idx) + UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx) { - return members.count(std::make_pair(method, param_idx)) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return members.count({method, param_idx}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; } /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */ - UniValue ArgToUniValue(const std::string& arg_value, const std::string& method, const std::string& param_name) + UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name) { - return membersByName.count(std::make_pair(method, param_name)) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; + return membersByName.count({method, param_name}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value; } }; @@ -254,13 +257,11 @@ static CRPCConvertTable rpcCvtTable; /** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) * as well as objects and arrays. */ -UniValue ParseNonRFCJSONValue(const std::string& strVal) +UniValue ParseNonRFCJSONValue(std::string_view raw) { - UniValue jVal; - if (!jVal.read(std::string("[")+strVal+std::string("]")) || - !jVal.isArray() || jVal.size()!=1) - throw std::runtime_error(std::string("Error parsing JSON: ") + strVal); - return jVal[0]; + UniValue parsed; + if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw)); + return parsed; } UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::string> &strParams) @@ -268,8 +269,8 @@ UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::s UniValue params(UniValue::VARR); for (unsigned int idx = 0; idx < strParams.size(); idx++) { - const std::string& strVal = strParams[idx]; - params.push_back(rpcCvtTable.ArgToUniValue(strVal, strMethod, idx)); + std::string_view value{strParams[idx]}; + params.push_back(rpcCvtTable.ArgToUniValue(value, strMethod, idx)); } return params; @@ -280,15 +281,15 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s UniValue params(UniValue::VOBJ); UniValue positional_args{UniValue::VARR}; - for (const std::string &s: strParams) { + for (std::string_view s: strParams) { size_t pos = s.find('='); if (pos == std::string::npos) { positional_args.push_back(rpcCvtTable.ArgToUniValue(s, strMethod, positional_args.size())); continue; } - std::string name = s.substr(0, pos); - std::string value = s.substr(pos+1); + std::string name{s.substr(0, pos)}; + std::string_view value{s.substr(pos+1)}; // Intentionally overwrite earlier named values with later ones as a // convenience for scripts and command line users that want to merge |