util/ref.h: “This implements a small subset of the functionality in C++17’s std::any class, and can be dropped when the project updates to C++17”. For accessing the contained object of a std::any instance, a helper template function AnyPtr is introduced (thanks to ryanofsky).
Concept ACK on removing no-longer needed abstraction. Currently failing to compile:
0 CXX wallet/libbitcoin_wallet_a-coincontrol.o
1In file included from /usr/include/c++/9/bits/move.h:55,
2 from /usr/include/c++/9/bits/nested_exception.h:40,
3 from /usr/include/c++/9/exception:144,
4 from /usr/include/c++/9/new:40,
5 from /usr/include/c++/9/any:37,
6 from ./rpc/request.h:9,
7 from ./rpc/server.h:10,
8 from rpc/server.cpp:6:
9/usr/include/c++/9/type_traits: In instantiation of ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’:
10/usr/include/c++/9/type_traits:150:27: required from ‘constexpr const bool std::__and_v<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’
11/usr/include/c++/9/any:192:27: required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> = <missing>]’
12/usr/include/c++/9/type_traits:883:12: required from ‘struct std::is_constructible<JSONRPCRequest, const JSONRPCRequest&>’
13/usr/include/c++/9/type_traits:901:12: required from ‘struct std::__is_copy_constructible_impl<JSONRPCRequest, true>’
14/usr/include/c++/9/type_traits:907:12: required from ‘struct std::is_copy_constructible<JSONRPCRequest>’
15/usr/include/c++/9/type_traits:131:12: required from ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >’
16/usr/include/c++/9/any:181:58: required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous>, typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous> = <missing>; typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> = <missing>]’
17rpc/server.cpp:91:32: required from here
18/usr/include/c++/9/type_traits:136:12: error: incomplete type ‘std::is_copy_constructible<JSONRPCRequest>’ used in nested name specifier
19 136 | struct __and_<_B1, _B2, _B3, _Bn...>
20 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
21/usr/include/c++/9/type_traits: In instantiation of ‘constexpr const bool std::__and_v<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’:
22/usr/include/c++/9/any:192:27: required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<__and_v<std::is_copy_constructible<_Tp>, std::__not_<std::is_constructible<_Tp, _ValueType&&> >, std::__not_<std::__is_in_place_type<_Tp> > >, bool>::type <anonymous> = <missing>]’
23/usr/include/c++/9/type_traits:883:12: required from ‘struct std::is_constructible<JSONRPCRequest, const JSONRPCRequest&>’
24/usr/include/c++/9/type_traits:901:12: required from ‘struct std::__is_copy_constructible_impl<JSONRPCRequest, true>’
25/usr/include/c++/9/type_traits:907:12: required from ‘struct std::is_copy_constructible<JSONRPCRequest>’
26/usr/include/c++/9/type_traits:131:12: required from ‘struct std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >’
27/usr/include/c++/9/any:181:58: required by substitution of ‘template<class _ValueType, class _Tp, class _Mgr, typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous>, typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> > std::any::any(_ValueType&&) [with _ValueType = const JSONRPCRequest&; _Tp = JSONRPCRequest; _Mgr = std::any::_Manager_external<JSONRPCRequest>; typename std::enable_if<std::__and_<std::is_copy_constructible<_Tp>, std::is_constructible<_Tp, _ValueType&&> >::value, bool>::type <anonymous> = <missing>; typename std::enable_if<(! std::__is_in_place_type<_Tp>::value), bool>::type <anonymous> = <missing>]’
28rpc/server.cpp:91:32: required from here
29/usr/include/c++/9/type_traits:150:27: error: ‘value’ is not a member of ‘std::__and_<std::is_copy_constructible<JSONRPCRequest>, std::__not_<std::is_constructible<JSONRPCRequest, const JSONRPCRequest&> >, std::__not_<std::__is_in_place_type<JSONRPCRequest> > >’
30 150 | inline constexpr bool __and_v = __and_<_Bn...>::value;
31 | ^~~~~~~
32make[2]: *** [Makefile:8807: rpc/libbitcoin_server_a-server.o] Error 1
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Meh, there seems to be a problem with the (default) copy-constructor of class JSONRPCRequest that is invoked in CRPCTable::help:
0... (..., const JSONRPCRequest& helpreq) {
1 ...
2 JSONRPCRequest jreq(helpreq);
3 ...
4}
No idea why this compiles locally with clang11 and obviously also for the CI jobs for MacOS and ARM.
104+ try {
105+ NodeContext& node_context = std::any_cast<std::reference_wrapper<NodeContext>>(context);
106+ node = &node_context;
107+ } catch (const std::bad_any_cast&) {
108+ node = nullptr;
109+ }
In commit “refactor: replace util::Ref by std::any (C++17)” (c6686a3a582670618b685e69bdfa2ba893b5ce92)
This change and other changes in this commit seems pretty obscure and verbose. It would be nice to define a helper function so these 7 lines could be just be replaced with:
0const NodeContext* node = util::AnyPtr<NodeContext>(context);
Helper could go in util/system.h and look like:
0template<typename T>
1T* AnyPtr(const std::any& any)
2{
3 try {
4 return std::any_cast<T*>(any);
5 } catch (const std::bad_any_cast&) {
6 return nullptr;
7 }
8}
Also I think it would be better to just assign context = &node instead of context = std::ref(node). I think the only reason you would use reference_wrappers instead of pointers is when you want non-nullable references, but std::any is inherently nullable so combining it with std::reference_wrapper just makes things complicated for no benefit.
std::any access helper and using pointers instead of references are great ideas, thanks! Looks much cleaner now, with the clunky try/catch-construct deduplicated and hidden in util/system.h.
AnyPtr to access std::any contents and generally assigned pointers to std::any instances instead of references. Code looks much cleaner now.
87@@ -87,7 +88,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
88 vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
89 sort(vCommands.begin(), vCommands.end());
90
91- JSONRPCRequest jreq(helpreq);
92+ JSONRPCRequest jreq(&helpreq);
In commit “refactor: replace util::Ref by std::any (C++17)” (feb55dde39e59a2709ecefff848a1d8eb9fe69ca)
There’s a bug here ~and this change should be reverted~. This line is supposed to create jreq as a copy of helpreq using the JSONRPCRequest copy constructor. Now it creates a new jreq request using the helpreq object as an embedded request context.
I’m not sure if this is a new bug, or this bug was present previously (even previously util::Ref constructor might have take precedence over copy constructor), but passing &helpreq here is not intended.
EDIT: Instead of reverting this, would suggest changing it to:
0JSONRPCRequest jreq = helpreq;
This would be more clear and prevent the context constructor from being called (since it’s marked explicit).
34@@ -38,14 +35,14 @@ class JSONRPCRequest
35 std::string URI;
36 std::string authUser;
37 std::string peerAddr;
38- const util::Ref& context;
39+ const std::any& context;
40
41- explicit JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
42+ explicit JSONRPCRequest(const std::any& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
In commit “refactor: replace util::Ref by std::any (C++17)” (feb55dde39e59a2709ecefff848a1d8eb9fe69ca)
This turns out to be dangerous because in some cases like JSONRPCRequest jreq(helpreq); this constructor will take precedence over the copy constructor.
~Would suggest changing this only to accept pointer arguments so it won’t be accidentally invoked trying to create a copy:~
0template<typename T>
1explicit JSONRPCRequest(T* context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
EDIT: Struck out suggestion above. It doesn’t work because the JSONRPCRequest needs a reference to a long-lived std::any object and can’t use a internal one. I’m not actually sure why this is the case, and think it may be worth simplifying in the future, but this would be a bigger change. Can disregard this comment.
506+ * Helper function to access the contained object of a std::any instance.
507+ * Returns a pointer to the object if passed instance has a value and the type
508+ * matches, nullptr otherwise.
509+ */
510+template<typename T>
511+T* AnyPtr(const std::any& any)
Concept NACK this implementation.
You should label this function as noexcept and use the noexcept versions of any_cast which will already return you a nullptr. See variants 4 and 5 https://en.cppreference.com/w/cpp/utility/any/any_cast. These are implemented AFAIK internally without using exceptions.
Exceptions have a non negligible performance overhead if hitting the catch case we may want to avoid.
You could – if you want – introduce a try/catch variant that is called AnyPtrLikely that uses try/catch where we’re already pretty sure that we’ll succeed, but that seems like over optimizing.
Oops, sorry. I didn’t know about the noexcept casts. This can be simplified to:
0template<typename T>
1T* AnyPtr(const std::any& any)
2{
3 T* const* ptr = std::any_cast<T*>(&any);
4 return ptr ? *ptr : nullptr;
5}
I don’t think this is used in any performance critical code, but no reason this needs to be slower and more complicated.
std::any_cast. But then I guess we can as well directly use that instead of introducing AnyPtr? This would be in effect just another name for the same purpose.
Good catch, I somehow missed that there was a noexcept version of
std::any_cast. But then I guess we can as well directly use that instead of introducing AnyPtr? This would be in effect just another name for the same purpose.
My preference would be to keep AnyPtr instead of introducing NodeContext* const* temporary variables because I think the two different ways these could be null could be confusing and cause non-obvious bugs, but feel free to take any approach.
using any_cast bare is a code smell IMO because it can arbitrarily become the non noexcept version :(, the wrapper makes the version guaranteed. If anything, you could declare it as inline so there is no overhead? (idk about template + inline…)
I think that’s what @ryanofsky is saying.
Another batch of simplifications you could consider. There’s no need really to require request objects to have contexts assigned.
0diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
1index 87c63593051..7d246325e44 100644
2--- a/src/bitcoind.cpp
3+++ b/src/bitcoind.cpp
4@@ -131,7 +131,7 @@ static bool AppInit(int argc, char* argv[])
5 // If locking the data directory failed, exit immediately
6 return false;
7 }
8- fRet = AppInitInterfaces(node) && AppInitMain(context, node);
9+ fRet = AppInitInterfaces(node) && AppInitMain(node);
10 }
11 catch (const std::exception& e) {
12 PrintExceptionContinue(&e, "AppInit()");
13diff --git a/src/httprpc.cpp b/src/httprpc.cpp
14index d33e671ee42..cd45eccc4ed 100644
15--- a/src/httprpc.cpp
16+++ b/src/httprpc.cpp
17@@ -159,7 +159,8 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
18 return false;
19 }
20
21- JSONRPCRequest jreq(context);
22+ JSONRPCRequest jreq;
23+ jreq.context = context;
24 jreq.peerAddr = req->GetPeer().ToString();
25 if (!RPCAuthorized(authHeader.second, jreq.authUser)) {
26 LogPrintf("ThreadRPCServer incorrect password attempt from %s\n", jreq.peerAddr);
27@@ -294,7 +295,7 @@ bool StartHTTPRPC(const std::any& context)
28 if (!InitRPCAuthentication())
29 return false;
30
31- auto handle_rpc = [&context](HTTPRequest* req, const std::string&) { return HTTPReq_JSONRPC(context, req); };
32+ auto handle_rpc = [context](HTTPRequest* req, const std::string&) { return HTTPReq_JSONRPC(context, req); };
33 RegisterHTTPHandler("/", true, handle_rpc);
34 if (g_wallet_init_interface.HasWalletSupport()) {
35 RegisterHTTPHandler("/wallet/", false, handle_rpc);
36diff --git a/src/init.cpp b/src/init.cpp
37index efb92802126..c166af10f1f 100644
38--- a/src/init.cpp
39+++ b/src/init.cpp
40@@ -785,7 +785,7 @@ static bool InitSanityCheck()
41 return true;
42 }
43
44-static bool AppInitServers(const std::any& context, NodeContext& node)
45+static bool AppInitServers(NodeContext& node)
46 {
47 const ArgsManager& args = *Assert(node.args);
48 RPCServer::OnStarted(&OnRPCStarted);
49@@ -794,9 +794,9 @@ static bool AppInitServers(const std::any& context, NodeContext& node)
50 return false;
51 StartRPC();
52 node.rpc_interruption_point = RpcInterruptionPoint;
53- if (!StartHTTPRPC(context))
54+ if (!StartHTTPRPC(&node))
55 return false;
56- if (args.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST(context);
57+ if (args.GetBoolArg("-rest", DEFAULT_REST_ENABLE)) StartREST(&node);
58 StartHTTPServer();
59 return true;
60 }
61@@ -1274,7 +1274,7 @@ bool AppInitInterfaces(NodeContext& node)
62 return true;
63 }
64
65-bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
66+bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
67 {
68 const ArgsManager& args = *Assert(node.args);
69 const CChainParams& chainparams = Params();
70@@ -1379,7 +1379,7 @@ bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAn
71 */
72 if (args.GetBoolArg("-server", false)) {
73 uiInterface.InitMessage_connect(SetRPCWarmupStatus);
74- if (!AppInitServers(context, node))
75+ if (!AppInitServers(node))
76 return InitError(_("Unable to start HTTP server. See debug log for details."));
77 }
78
79diff --git a/src/init.h b/src/init.h
80index fe1c66202bf..d8fcb1e0ce3 100644
81--- a/src/init.h
82+++ b/src/init.h
83@@ -59,7 +59,7 @@ bool AppInitInterfaces(NodeContext& node);
84 * [@note](/bitcoin-bitcoin/contributor/note/) This should only be done after daemonization. Call Shutdown() if this function fails.
85 * [@pre](/bitcoin-bitcoin/contributor/pre/) Parameters should be parsed and config file should be read, AppInitLockDataDirectory should have been called.
86 */
87-bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr);
88+bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr);
89
90 /**
91 * Register all arguments with the ArgsManager
92diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
93index 8cf0dee5b4e..7d195688032 100644
94--- a/src/node/interfaces.cpp
95+++ b/src/node/interfaces.cpp
96@@ -76,7 +76,7 @@ public:
97 }
98 bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
99 {
100- return AppInitMain(m_context_ref, *m_context, tip_info);
101+ return AppInitMain(*m_context, tip_info);
102 }
103 void appShutdown() override
104 {
105@@ -218,7 +218,8 @@ public:
106 CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
107 UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
108 {
109- JSONRPCRequest req(m_context_ref);
110+ JSONRPCRequest req;
111+ req.context = m_context;
112 req.params = params;
113 req.strMethod = command;
114 req.URI = uri;
115@@ -287,14 +288,8 @@ public:
116 void setContext(NodeContext* context) override
117 {
118 m_context = context;
119- if (context) {
120- m_context_ref = context;
121- } else {
122- m_context_ref.reset();
123- }
124 }
125 NodeContext* m_context{nullptr};
126- std::any m_context_ref;
127 };
128
129 bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock, const CChain& active)
130diff --git a/src/rest.cpp b/src/rest.cpp
131index 452435f3e92..e2af5128121 100644
132--- a/src/rest.cpp
133+++ b/src/rest.cpp
134@@ -317,7 +317,8 @@ static bool rest_chaininfo(const std::any& context, HTTPRequest* req, const std:
135
136 switch (rf) {
137 case RetFormat::JSON: {
138- JSONRPCRequest jsonRequest(context);
139+ JSONRPCRequest jsonRequest;
140+ jsonRequest.context = context;
141 jsonRequest.params = UniValue(UniValue::VARR);
142 UniValue chainInfoObject = getblockchaininfo().HandleRequest(jsonRequest);
143 std::string strJSON = chainInfoObject.write() + "\n";
144@@ -687,7 +688,7 @@ static const struct {
145 void StartREST(const std::any& context)
146 {
147 for (const auto& up : uri_prefixes) {
148- auto handler = [&context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };
149+ auto handler = [context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };
150 RegisterHTTPHandler(up.prefix, false, handler);
151 }
152 }
153diff --git a/src/rpc/request.h b/src/rpc/request.h
154index 82d8fdc63ae..7a78d382cc9 100644
155--- a/src/rpc/request.h
156+++ b/src/rpc/request.h
157@@ -35,18 +35,7 @@ public:
158 std::string URI;
159 std::string authUser;
160 std::string peerAddr;
161- const std::any& context;
162-
163- explicit JSONRPCRequest(const std::any& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
164-
165- //! Initializes request information from another request object and the
166- //! given context. The implementation should be updated if any members are
167- //! added or removed above.
168- JSONRPCRequest(const JSONRPCRequest& other, const std::any& context)
169- : id(other.id), strMethod(other.strMethod), params(other.params), fHelp(other.fHelp), URI(other.URI),
170- authUser(other.authUser), peerAddr(other.peerAddr), context(context)
171- {
172- }
173+ std::any context;
174
175 void parse(const UniValue& valRequest);
176 };
177diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
178index 3d5216055ce..f16adf96d3e 100644
179--- a/src/rpc/server.cpp
180+++ b/src/rpc/server.cpp
181@@ -88,7 +88,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
182 vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
183 sort(vCommands.begin(), vCommands.end());
184
185- JSONRPCRequest jreq(&helpreq);
186+ JSONRPCRequest jreq(helpreq);
187 jreq.fHelp = true;
188 jreq.params = UniValue();
189
190diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
191index 4ffc2478696..d2749c89599 100644
192--- a/src/test/rpc_tests.cpp
193+++ b/src/test/rpc_tests.cpp
194@@ -33,8 +33,8 @@ UniValue RPCTestingSetup::CallRPC(std::string args)
195 boost::split(vArgs, args, boost::is_any_of(" \t"));
196 std::string strMethod = vArgs[0];
197 vArgs.erase(vArgs.begin());
198- std::any context{&m_node};
199- JSONRPCRequest request(context);
200+ JSONRPCRequest request;
201+ request.context = &m_node;
202 request.strMethod = strMethod;
203 request.params = RPCConvertValues(strMethod, vArgs);
204 request.fHelp = false;
205diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
206index c91ede94f95..46533d8704c 100644
207--- a/src/wallet/interfaces.cpp
208+++ b/src/wallet/interfaces.cpp
209@@ -514,7 +514,9 @@ public:
210 {
211 for (const CRPCCommand& command : GetWalletRPCCommands()) {
212 m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
213- return command.actor({request, &m_context}, result, last_handler);
214+ JSONRPCRequest wrapped = request;
215+ wrapped.context = &m_context;
216+ return command.actor(wrapped, result, last_handler);
217 }, command.argNames, command.unique_id);
218 m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
219 }
220diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
221index e48f7b4d4f5..a9c20921abd 100644
222--- a/src/wallet/test/wallet_tests.cpp
223+++ b/src/wallet/test/wallet_tests.cpp
224@@ -213,8 +213,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
225 key.pushKV("timestamp", newTip->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1);
226 key.pushKV("internal", UniValue(true));
227 keys.push_back(key);
228- std::any context;
229- JSONRPCRequest request(context);
230+ JSONRPCRequest request;
231 request.params.setArray();
232 request.params.push_back(keys);
233
234@@ -265,8 +264,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
235 AddWallet(wallet);
236 wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
237 }
238- std::any context;
239- JSONRPCRequest request(context);
240+ JSONRPCRequest request;
241 request.params.setArray();
242 request.params.push_back(backup_file);
243
244@@ -281,8 +279,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
245 LOCK(wallet->cs_wallet);
246 wallet->SetupLegacyScriptPubKeyMan();
247
248- std::any context;
249- JSONRPCRequest request(context);
250+ JSONRPCRequest request;
251 request.params.setArray();
252 request.params.push_back(backup_file);
253 AddWallet(wallet);
Force-pushed with feedback from @JeremyRubin (https://github.com/bitcoin/bitcoin/pull/21366#discussion_r588921151) and @ryanofsky (https://github.com/bitcoin/bitcoin/pull/21366#discussion_r588923880) taken into account: AnyPtr uses now the variant of std::any_cast that doesn’t use exceptions. Also updated the PR description which still referred to the changeset without the AnyPtr helper and the exception variant of std::any_cast.
Another batch of simplifications you could consider. There’s no need really to require request objects to have contexts assigned.
Good idea. I think those changes are out of scope for this PR, as the proposed simplifications are not connected to the util::Ref/std::any replacements. Happy to open a follow-up PR though or review if someone else does.
Concept ACK
+89 −155 – very nice! :)
37@@ -38,7 +38,6 @@
38 #include <uint256.h>
39 #include <univalue.h>
40 #include <util/check.h>
41-#include <util/ref.h>
#include <any> ?
74@@ -73,18 +75,18 @@ static bool RESTERR(HTTPRequest* req, enum HTTPStatusCode status, std::string me
75 * context is not found.
76 * @returns Pointer to the node context or nullptr if not found.
77 */
78-static NodeContext* GetNodeContext(const util::Ref& context, HTTPRequest* req)
79+static NodeContext* GetNodeContext(const std::any& context, HTTPRequest* req)
80 {
81- NodeContext* node = context.Has<NodeContext>() ? &context.Get<NodeContext>() : nullptr;
82- if (!node) {
83+ NodeContext* node_context = util::AnyPtr<NodeContext>(context);
nit: Could be more concise without losing much details:
0 auto node_context = util::AnyPtr<NodeContext>(context);
(Here and in other places when util::AnyPtr<T> is used)
14@@ -15,6 +15,7 @@
15 #include <boost/algorithm/string/split.hpp>
16 #include <boost/signals2/signal.hpp>
17
18+#include <any>
#include <any> here.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
re-ACK 916ab0195d567fd0a9097045e73a6654c453adea, with command
0$ git range-diff master 39069499a61d7c2cab8594162c99bf00ea2ac88b 916ab0195d567fd0a9097045e73a6654c453adea
verified that since my previous review only rebased and implemented suggestions.
auto. I might have used const auto* and auto* instead of plain auto because I think the qualifiers are useful, but this is all good.