refactor: Return std::optional from GetNameProxy/GetProxy#34741
refactor: Return std::optional from GetNameProxy/GetProxy#34741maflcko wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, 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. |
|
Wouldn't util::Result be a better fit? But since IsValid is already there, why introduce a second way to indicate none? |
Why? Result is primarily intended for high-level errors that should be passed to the user, possibly even translated. I think this is low-level enough to not need translated error strings, or error messages at all. Also, adding error strings won't be possible in a refactor-only pull anyway.
It is what other new code is using. You can use |
src/qt/clientmodel.cpp
Outdated
| const auto ipv4 = m_node.getProxy((Network)1); | ||
| const auto ipv6 = m_node.getProxy((Network)2); |
There was a problem hiding this comment.
nit: since this section is already being refactored, it might be a good opportunity to replace the magic numbers with Network::NET_IPV4 / Network::NET_IPV6 for clarity.
| const auto ipv4 = m_node.getProxy((Network)1); | |
| const auto ipv6 = m_node.getProxy((Network)2); | |
| const auto ipv4 = m_node.getProxy(Network::NET_IPV4); | |
| const auto ipv6 = m_node.getProxy(Network::NET_IPV6); |
Currently the getters have a mutable reference as inout param and return a bool to indicate success. This is confusing, because the success bool is redundant with the
IsValid()state on the proxy object.So in theory, the functions could reset the mutable proxy object to an invalid state and return
void.However, this would also be confusing, because devs can forget to check
IsValid().Fix all issues by using
std::optional<Proxy>, where devs no longer have to checkIsValid()manually, or a separate bool. Note that new code in the repo is already usingstd::optional<Proxy>, seegit grep 'std::optional<Proxy>' bitcoin-core/master. Also,std::optional<Proxy>will enforce checking at compile time, whereas callingProxy::IsValidis not enforced.