rpc: Use the return value of GetProxy(...) in GetNetworksInfo(). Mark GetProxy(...) with [[nodiscard]]. #15215

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:getnetworkinfo-getproxy changing 2 files +9 −5

practicalswift commented at 2:40 PM on January 20, 2019: contributor
- Use the return value of GetProxy(...) in GetNetworksInfo().
- Mark GetProxy(...) with [[nodiscard]].
fanquake added the label RPC/REST/ZMQ on Jan 20, 2019
fanquake added the label P2P on Jan 20, 2019

in src/rpc/net.cpp:429 in e0fd3c8295 outdated

 428 |          obj.pushKV("reachable", IsReachable(network));
 429 | -        obj.pushKV("proxy", proxy.IsValid() ? proxy.proxy.ToStringIPPort() : std::string());
 430 | -        obj.pushKV("proxy_randomize_credentials", proxy.randomize_credentials);
 431 | +
 432 | +        proxyType proxy;
 433 | +        bool useProxy = GetProxy(network, proxy);

promag commented at 2:04 PM on January 21, 2019:

nit, use_proxy.

promag commented at 2:05 PM on January 21, 2019: member

utACK
rpc: Use the return value of GetProxy(...) in GetNetworksInfo(). Mark GetProxy(...) with [[nodiscard]]. 73e4d446e0
practicalswift force-pushed on Jan 21, 2019
practicalswift commented at 2:15 PM on January 21, 2019: contributor

@promag Thanks for reviewing! Feedback addressed. Please re-review :-)
in src/netbase.h:44 in 73e4d446e0
```
  40 | @@ -40,7 +41,7 @@ class proxyType
  41 |  enum Network ParseNetwork(std::string net);
  42 |  std::string GetNetworkName(enum Network net);
  43 |  bool SetProxy(enum Network net, const proxyType &addrProxy);
  44 | -bool GetProxy(enum Network net, proxyType &proxyInfoOut);
  45 | +NODISCARD bool GetProxy(enum Network net, proxyType &proxyInfoOut);
```
laanwj commented at 3:09 PM on February 12, 2019:

If the boolean is only there to signify the structure being there or not, what would be cool imo is to return Option<proxyType>, e.g.

#include <optional.h> ... Optional<proxyType> GetProxy(enum Network net);

No need for NODISCARD either.
laanwj commented at 3:37 PM on February 12, 2019:

AHHH this gets into trouble in OptionsDialog::updateDefaultProxyNets which seems to rely on the struct even if the return value and proxy isn't valid (it assumes it gets default values from there?).
laanwj commented at 4:06 PM on February 12, 2019: member

Here's my branch that uses Optional: https://github.com/laanwj/bitcoin/tree/2019_02_optional

The only thing I'm not sure about is whether the options dialog change is handled correctly, what the proxy string should be in case of an nonexistent/invalid proxy.
practicalswift commented at 9:41 AM on February 17, 2019: contributor

@laanwj Your solution looks better. I'm closing this PR: please consider submitting your PR instead.
practicalswift closed this on Feb 17, 2019
laanwj commented at 12:26 PM on February 17, 2019: member

Thanks! I'll try to get back to it after 0.18.
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels