Merge bitcoin/bitcoin#34938: refactor: Return std::optional over bool+mut&

fabab69e9e refactor: Return std::optional from ParseDouble (MarcoFalke)
fa0a09441d refactor: Return std::optional from GetWalletNameFromJSONRPCRequest (MarcoFalke)
fafb0c4cbe refactor: Return std::optional from GetLogCategory (MarcoFalke)

Pull request description:

  Using a bool to indicate whether a mutable in-out param was written to is fine in legacy code, but otherwise confusing and brittle:

  * Sometimes the in-out-param is written to, even when the function returns `false`, like in `ParseDouble`.
  * Call sites must manually check the return value

  Fix those issues by returning `std::optional<_>` from `ParseDouble` (and a few other functions).

  This refactor is a style cleanup and does not change any behavior.

ACKs for top commit:
  stickies-v:
    re-ACK fabab69e9e
  hodlinator:
    crACK fabab69e9e
  rkrux:
    code review ACK fabab69e9e

Tree-SHA512: e27a27174e9d2200da8b0ca9b6cc056e94d2b25e6332975f1ad660ee85b02680a65ac93b2ed29f10da0ae5f6dc8395bddc9e973a3f925c68a0c8116c9282ea09

src/logging.cpp

@@ -127,10 +127,11 @@ void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)
 
 bool BCLog::Logger::EnableCategory(std::string_view str)
 {
-    BCLog::LogFlags flag;
-    if (!GetLogCategory(flag, str)) return false;
-    EnableCategory(flag);
-    return true;
+    if (const auto flag{GetLogCategory(str)}) {
+        EnableCategory(*flag);
+        return true;
+    }
+    return false;
 }
 
 void BCLog::Logger::DisableCategory(BCLog::LogFlags flag)
@@ -140,10 +141,11 @@ void BCLog::Logger::DisableCategory(BCLog::LogFlags flag)
 
 bool BCLog::Logger::DisableCategory(std::string_view str)
 {
-    BCLog::LogFlags flag;
-    if (!GetLogCategory(flag, str)) return false;
-    DisableCategory(flag);
-    return true;
+    if (const auto flag{GetLogCategory(str)}) {
+        DisableCategory(*flag);
+        return true;
+    }
+    return false;
 }
 
 bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const
@@ -217,18 +219,16 @@ static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_
     }(LOG_CATEGORIES_BY_STR)
 };
 
-bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str)
+std::optional<BCLog::LogFlags> GetLogCategory(std::string_view str)
 {
     if (str.empty() || str == "1" || str == "all") {
-        flag = BCLog::ALL;
-        return true;
+        return BCLog::ALL;
     }
     auto it = LOG_CATEGORIES_BY_STR.find(str);
     if (it != LOG_CATEGORIES_BY_STR.end()) {
-        flag = it->second;
-        return true;
+        return it->second;
     }
-    return false;
+    return std::nullopt;
 }
 
 std::string BCLog::Logger::LogLevelToStr(BCLog::Level level)
@@ -592,14 +592,14 @@ bool BCLog::Logger::SetLogLevel(std::string_view level_str)
 
 bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::string_view level_str)
 {
-    BCLog::LogFlags flag;
-    if (!GetLogCategory(flag, category_str)) return false;
+    const auto flag{GetLogCategory(category_str)};
+    if (!flag) return false;
 
     const auto level = GetLogLevel(level_str);
     if (!level.has_value() || level.value() > MAX_USER_SETABLE_SEVERITY_LEVEL) return false;
 
     STDLOCK(m_cs);
-    m_category_log_levels[flag] = level.value();
+    m_category_log_levels[*flag] = level.value();
     return true;
 }
 

src/logging.h

@@ -20,6 +20,7 @@
 #include <functional>
 #include <list>
 #include <memory>
+#include <optional>
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -296,7 +297,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
     return LogInstance().WillLogCategoryLevel(category, level);
 }
 
-/** Return true if str parses as a log category and set the flag */
-bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
+/// Return log flag if str parses as a log category.
+std::optional<BCLog::LogFlags> GetLogCategory(std::string_view str);
 
 #endif // BITCOIN_LOGGING_H

src/test/logging_tests.cpp

@@ -165,9 +165,8 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros_CategoryName, LogSetup)
     std::vector<std::pair<BCLog::LogFlags, std::string>> expected_category_names;
     const auto category_names = SplitString(concatenated_category_names, ',');
     for (const auto& category_name : category_names) {
-        BCLog::LogFlags category;
         const auto trimmed_category_name = TrimString(category_name);
-        BOOST_REQUIRE(GetLogCategory(category, trimmed_category_name));
+        const auto category{*Assert(GetLogCategory(trimmed_category_name))};
         expected_category_names.emplace_back(category, trimmed_category_name);
     }
 

src/univalue/lib/univalue_get.cpp

@@ -7,6 +7,7 @@
 
 #include <cstring>
 #include <locale>
+#include <optional>
 #include <sstream>
 #include <stdexcept>
 #include <string>
@@ -25,18 +26,20 @@ static bool ParsePrechecks(const std::string& str)
     return true;
 }
 
-bool ParseDouble(const std::string& str, double *out)
+std::optional<double> ParseDouble(const std::string& str)
 {
     if (!ParsePrechecks(str))
-        return false;
+        return std::nullopt;
     if (str.size() >= 2 && str[0] == '0' && str[1] == 'x') // No hexadecimal floats allowed
-        return false;
+        return std::nullopt;
     std::istringstream text(str);
     text.imbue(std::locale::classic());
     double result;
     text >> result;
-    if(out) *out = result;
-    return text.eof() && !text.fail();
+    if (!text.eof() || text.fail()) {
+        return std::nullopt;
+    }
+    return result;
 }
 }
 
@@ -68,10 +71,10 @@ const std::string& UniValue::get_str() const
 double UniValue::get_real() const
 {
     checkType(VNUM);
-    double retval;
-    if (!ParseDouble(getValStr(), &retval))
-        throw std::runtime_error("JSON double out of range");
-    return retval;
+    if (const auto retval{ParseDouble(getValStr())}) {
+        return *retval;
+    }
+    throw std::runtime_error("JSON double out of range");
 }
 
 const UniValue& UniValue::get_obj() const

src/wallet/rpc/util.cpp

@@ -32,14 +32,13 @@ bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) {
 
 std::string EnsureUniqueWalletName(const JSONRPCRequest& request, std::optional<std::string_view> wallet_name)
 {
-    std::string endpoint_wallet;
-    if (GetWalletNameFromJSONRPCRequest(request, endpoint_wallet)) {
+    if (auto endpoint_wallet{GetWalletNameFromJSONRPCRequest(request)}) {
         // wallet endpoint was used
-        if (wallet_name && *wallet_name != endpoint_wallet) {
+        if (wallet_name && *wallet_name != *endpoint_wallet) {
             throw JSONRPCError(RPC_INVALID_PARAMETER,
                 "The RPC endpoint wallet and the wallet name parameter specify different wallets");
         }
-        return endpoint_wallet;
+        return *endpoint_wallet;
     }
 
     // Not a wallet endpoint; parameter must be provided
@@ -51,14 +50,13 @@ std::string EnsureUniqueWalletName(const JSONRPCRequest& request, std::optional<
     return std::string{*wallet_name};
 }
 
-bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
+std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request)
 {
     if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) {
         // wallet endpoint was used
-        wallet_name = UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size()));
-        return true;
+        return UrlDecode(std::string_view{request.URI}.substr(WALLET_ENDPOINT_BASE.size()));
     }
-    return false;
+    return std::nullopt;
 }
 
 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
@@ -66,9 +64,8 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
     CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
     WalletContext& context = EnsureWalletContext(request.context);
 
-    std::string wallet_name;
-    if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
-        std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
+    if (auto wallet_name{GetWalletNameFromJSONRPCRequest(request)}) {
+        std::shared_ptr<CWallet> pwallet{GetWallet(context, *wallet_name)};
         if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
         return pwallet;
     }

src/wallet/rpc/util.h

@@ -39,7 +39,7 @@ static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "last
  * @return nullptr if no wallet should be used, or a pointer to the CWallet
  */
 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
-bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name);
+std::optional<std::string> GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request);
 /**
  * Ensures that a wallet name is specified across the endpoint and wallet_name.
  * Throws `RPC_INVALID_PARAMETER` if none or different wallet names are specified.