diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-06-09 17:17:58 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-06-09 17:18:04 -0400 |
commit | f8364df25070cea08f0fb5bbbb212f1ff72f9d21 (patch) | |
tree | 0f0a706e0b862b583234c743dd466e07b7c1c23f | |
parent | 221873e15874b5d5c02d674bcc982446adae6fa3 (diff) | |
parent | 6fe989054f0ad9308e8a25f7123d9e5dd67f1164 (diff) | |
download | bitcoin-f8364df25070cea08f0fb5bbbb212f1ff72f9d21.tar.xz |
Merge #19176: refactor: Error message bilingual_str consistency
6fe989054f0ad9308e8a25f7123d9e5dd67f1164 refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan)
425e7cb8cf6140e03802a96d2be9a8b4aa2e244a refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan)
77b79fa6ef60d363ca720cef5473f1a2c45099a3 refactor: Error message bilingual_str consistency (Wladimir J. van der Laan)
Pull request description:
A straightforward and hopefully uncontroversial refactor to improve consistency.
- Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`.
- Make all functions in `util/error.h` consistently return a `bilingual_str`. We've decided to use this as error message type so let's roll with it.
This has no functional changes: no messages are changed, no new translation messages are defined.
Also make a function static that can be static.
ACKs for top commit:
MarcoFalke:
ACK 6fe989054f0ad9308e8a25f7123d9e5dd67f1164 🔣
hebasto:
ACK 6fe989054f0ad9308e8a25f7123d9e5dd67f1164, tested on Linux Mint 19.3 (x86_64).
Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d
-rw-r--r-- | src/init.cpp | 12 | ||||
-rw-r--r-- | src/interfaces/node.cpp | 2 | ||||
-rw-r--r-- | src/interfaces/node.h | 2 | ||||
-rw-r--r-- | src/net_permissions.cpp | 22 | ||||
-rw-r--r-- | src/net_permissions.h | 6 | ||||
-rw-r--r-- | src/qt/bitcoin.cpp | 8 | ||||
-rw-r--r-- | src/rpc/util.cpp | 3 | ||||
-rw-r--r-- | src/test/fuzz/kitchen_sink.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/net_permissions.cpp | 5 | ||||
-rw-r--r-- | src/test/netbase_tests.cpp | 11 | ||||
-rw-r--r-- | src/util/error.cpp | 26 | ||||
-rw-r--r-- | src/util/error.h | 4 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 2 |
13 files changed, 57 insertions, 47 deletions
diff --git a/src/init.cpp b/src/init.cpp index 56d75c90c7..fd7c8d0f80 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1465,7 +1465,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node) if (Lookup(strAddr, addrLocal, GetListenPort(), fNameLookup) && addrLocal.IsValid()) AddLocal(addrLocal, LOCAL_MANUAL); else - return InitError(Untranslated(ResolveErrMsg("externalip", strAddr))); + return InitError(ResolveErrMsg("externalip", strAddr)); } // Read asmap file if configured @@ -1904,21 +1904,21 @@ bool AppInitMain(const util::Ref& context, NodeContext& node) for (const std::string& strBind : gArgs.GetArgs("-bind")) { CService addrBind; if (!Lookup(strBind, addrBind, GetListenPort(), false)) { - return InitError(Untranslated(ResolveErrMsg("bind", strBind))); + return InitError(ResolveErrMsg("bind", strBind)); } connOptions.vBinds.push_back(addrBind); } for (const std::string& strBind : gArgs.GetArgs("-whitebind")) { NetWhitebindPermissions whitebind; - std::string error; - if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(Untranslated(error)); + bilingual_str error; + if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error); connOptions.vWhiteBinds.push_back(whitebind); } for (const auto& net : gArgs.GetArgs("-whitelist")) { NetWhitelistPermissions subnet; - std::string error; - if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(Untranslated(error)); + bilingual_str error; + if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(error); connOptions.vWhitelistedRange.push_back(subnet); } diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp index bd1b36fea5..7995d8d7b0 100644 --- a/src/interfaces/node.cpp +++ b/src/interfaces/node.cpp @@ -56,7 +56,7 @@ namespace { class NodeImpl : public Node { public: - void initError(const std::string& message) override { InitError(Untranslated(message)); } + void initError(const bilingual_str& message) override { InitError(message); } bool parseParameters(int argc, const char* const argv[], std::string& error) override { return gArgs.ParseParameters(argc, argv, error); diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 0b7fb6736a..fad84789cc 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -45,7 +45,7 @@ public: virtual ~Node() {} //! Send init error. - virtual void initError(const std::string& message) = 0; + virtual void initError(const bilingual_str& message) = 0; //! Set command line arguments. virtual bool parseParameters(int argc, const char* const argv[], std::string& error) = 0; diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp index d76a512a6c..da09149856 100644 --- a/src/net_permissions.cpp +++ b/src/net_permissions.cpp @@ -16,8 +16,10 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{ "mempool (allow requesting BIP35 mempool contents)", }; +namespace { + // The parse the following format "perm1,perm2@xxxxxx" -bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, std::string& error) +bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, bilingual_str& error) { NetPermissionFlags flags = PF_NONE; const auto atSeparator = str.find('@'); @@ -48,7 +50,7 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY); else if (permission.length() == 0); // Allow empty entries else { - error = strprintf(_("Invalid P2P permission: '%s'").translated, permission); + error = strprintf(_("Invalid P2P permission: '%s'"), permission); return false; } } @@ -56,10 +58,12 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, } output = flags; - error = ""; + error = Untranslated(""); return true; } +} + std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags) { std::vector<std::string> strings; @@ -71,7 +75,7 @@ std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags) return strings; } -bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error) +bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermissions& output, bilingual_str& error) { NetPermissionFlags flags; size_t offset; @@ -84,17 +88,17 @@ bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermis return false; } if (addrBind.GetPort() == 0) { - error = strprintf(_("Need to specify a port with -whitebind: '%s'").translated, strBind); + error = strprintf(_("Need to specify a port with -whitebind: '%s'"), strBind); return false; } output.m_flags = flags; output.m_service = addrBind; - error = ""; + error = Untranslated(""); return true; } -bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error) +bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermissions& output, bilingual_str& error) { NetPermissionFlags flags; size_t offset; @@ -104,12 +108,12 @@ bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermis CSubNet subnet; LookupSubNet(net, subnet); if (!subnet.IsValid()) { - error = strprintf(_("Invalid netmask specified in -whitelist: '%s'").translated, net); + error = strprintf(_("Invalid netmask specified in -whitelist: '%s'"), net); return false; } output.m_flags = flags; output.m_subnet = subnet; - error = ""; + error = Untranslated(""); return true; } diff --git a/src/net_permissions.h b/src/net_permissions.h index d35c5ee0cd..e004067e75 100644 --- a/src/net_permissions.h +++ b/src/net_permissions.h @@ -10,6 +10,8 @@ #ifndef BITCOIN_NET_PERMISSIONS_H #define BITCOIN_NET_PERMISSIONS_H +struct bilingual_str; + extern const std::vector<std::string> NET_PERMISSIONS_DOC; enum NetPermissionFlags @@ -54,14 +56,14 @@ public: class NetWhitebindPermissions : public NetPermissions { public: - static bool TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error); + static bool TryParse(const std::string str, NetWhitebindPermissions& output, bilingual_str& error); CService m_service; }; class NetWhitelistPermissions : public NetPermissions { public: - static bool TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error); + static bool TryParse(const std::string str, NetWhitelistPermissions& output, bilingual_str& error); CSubNet m_subnet; }; diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 3e7a3e9271..2cfc0f7836 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -457,7 +457,7 @@ int GuiMain(int argc, char* argv[]) SetupUIArgs(); std::string error; if (!node->parseParameters(argc, argv, error)) { - node->initError(strprintf("Error parsing command line arguments: %s\n", error)); + node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error)); // Create a message box, because the gui has neither been created nor has subscribed to core signals QMessageBox::critical(nullptr, PACKAGE_NAME, // message can not be translated because translations have not been initialized @@ -498,13 +498,13 @@ int GuiMain(int argc, char* argv[]) /// 6. Determine availability of data directory and parse bitcoin.conf /// - Do not call GetDataDir(true) before this step finishes if (!CheckDataDirOption()) { - node->initError(strprintf("Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", ""))); + node->initError(strprintf(Untranslated("Specified data directory \"%s\" does not exist.\n"), gArgs.GetArg("-datadir", ""))); QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", "")))); return EXIT_FAILURE; } if (!node->readConfigFiles(error)) { - node->initError(strprintf("Error reading configuration file: %s\n", error)); + node->initError(strprintf(Untranslated("Error reading configuration file: %s\n"), error)); QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error))); return EXIT_FAILURE; @@ -520,7 +520,7 @@ int GuiMain(int argc, char* argv[]) try { node->selectParams(gArgs.GetChainName()); } catch(std::exception &e) { - node->initError(strprintf("%s\n", e.what())); + node->initError(Untranslated(strprintf("%s\n", e.what()))); QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what())); return EXIT_FAILURE; } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 39bf05fbbd..e7afef4cac 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -10,6 +10,7 @@ #include <tinyformat.h> #include <util/strencodings.h> #include <util/string.h> +#include <util/translation.h> #include <tuple> @@ -285,7 +286,7 @@ UniValue JSONRPCTransactionError(TransactionError terr, const std::string& err_s if (err_string.length() > 0) { return JSONRPCError(RPCErrorFromTransactionError(terr), err_string); } else { - return JSONRPCError(RPCErrorFromTransactionError(terr), TransactionErrorString(terr)); + return JSONRPCError(RPCErrorFromTransactionError(terr), TransactionErrorString(terr).original); } } diff --git a/src/test/fuzz/kitchen_sink.cpp b/src/test/fuzz/kitchen_sink.cpp index af6dc71322..82cbc00a3a 100644 --- a/src/test/fuzz/kitchen_sink.cpp +++ b/src/test/fuzz/kitchen_sink.cpp @@ -7,6 +7,7 @@ #include <test/fuzz/fuzz.h> #include <test/fuzz/util.h> #include <util/error.h> +#include <util/translation.h> #include <cstdint> #include <vector> diff --git a/src/test/fuzz/net_permissions.cpp b/src/test/fuzz/net_permissions.cpp index c071283467..ae531f4462 100644 --- a/src/test/fuzz/net_permissions.cpp +++ b/src/test/fuzz/net_permissions.cpp @@ -6,6 +6,7 @@ #include <test/fuzz/FuzzedDataProvider.h> #include <test/fuzz/fuzz.h> #include <test/fuzz/util.h> +#include <util/translation.h> #include <cassert> #include <cstdint> @@ -29,7 +30,7 @@ void test_one_input(const std::vector<uint8_t>& buffer) static_cast<NetPermissionFlags>(fuzzed_data_provider.ConsumeIntegral<uint32_t>()); NetWhitebindPermissions net_whitebind_permissions; - std::string error_net_whitebind_permissions; + bilingual_str error_net_whitebind_permissions; if (NetWhitebindPermissions::TryParse(s, net_whitebind_permissions, error_net_whitebind_permissions)) { (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags); (void)NetPermissions::AddFlag(net_whitebind_permissions.m_flags, net_permission_flags); @@ -39,7 +40,7 @@ void test_one_input(const std::vector<uint8_t>& buffer) } NetWhitelistPermissions net_whitelist_permissions; - std::string error_net_whitelist_permissions; + bilingual_str error_net_whitelist_permissions; if (NetWhitelistPermissions::TryParse(s, net_whitelist_permissions, error_net_whitelist_permissions)) { (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags); (void)NetPermissions::AddFlag(net_whitelist_permissions.m_flags, net_permission_flags); diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index 2e1972cc3f..d0ec401f9d 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -6,6 +6,7 @@ #include <netbase.h> #include <test/util/setup_common.h> #include <util/strencodings.h> +#include <util/translation.h> #include <string> @@ -325,15 +326,15 @@ BOOST_AUTO_TEST_CASE(netbase_parsenetwork) BOOST_AUTO_TEST_CASE(netpermissions_test) { - std::string error; + bilingual_str error; NetWhitebindPermissions whitebindPermissions; NetWhitelistPermissions whitelistPermissions; // Detect invalid white bind BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error)); - BOOST_CHECK(error.find("Cannot resolve -whitebind address") != std::string::npos); + BOOST_CHECK(error.original.find("Cannot resolve -whitebind address") != std::string::npos); BOOST_CHECK(!NetWhitebindPermissions::TryParse("127.0.0.1", whitebindPermissions, error)); - BOOST_CHECK(error.find("Need to specify a port with -whitebind") != std::string::npos); + BOOST_CHECK(error.original.find("Need to specify a port with -whitebind") != std::string::npos); BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error)); // If no permission flags, assume backward compatibility @@ -377,11 +378,11 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) // Detect invalid flag BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK(error.find("Invalid P2P permission") != std::string::npos); + BOOST_CHECK(error.original.find("Invalid P2P permission") != std::string::npos); // Check whitelist error BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, error)); - BOOST_CHECK(error.find("Invalid netmask specified in -whitelist") != std::string::npos); + BOOST_CHECK(error.original.find("Invalid netmask specified in -whitelist") != std::string::npos); // Happy path for whitelist parsing BOOST_CHECK(NetWhitelistPermissions::TryParse("noban@1.2.3.4", whitelistPermissions, error)); diff --git a/src/util/error.cpp b/src/util/error.cpp index 72a6e87cde..c4d9ffd037 100644 --- a/src/util/error.cpp +++ b/src/util/error.cpp @@ -8,37 +8,37 @@ #include <util/system.h> #include <util/translation.h> -std::string TransactionErrorString(const TransactionError err) +bilingual_str TransactionErrorString(const TransactionError err) { switch (err) { case TransactionError::OK: - return "No error"; + return Untranslated("No error"); case TransactionError::MISSING_INPUTS: - return "Missing inputs"; + return Untranslated("Missing inputs"); case TransactionError::ALREADY_IN_CHAIN: - return "Transaction already in block chain"; + return Untranslated("Transaction already in block chain"); case TransactionError::P2P_DISABLED: - return "Peer-to-peer functionality missing or disabled"; + return Untranslated("Peer-to-peer functionality missing or disabled"); case TransactionError::MEMPOOL_REJECTED: - return "Transaction rejected by AcceptToMemoryPool"; + return Untranslated("Transaction rejected by AcceptToMemoryPool"); case TransactionError::MEMPOOL_ERROR: - return "AcceptToMemoryPool failed"; + return Untranslated("AcceptToMemoryPool failed"); case TransactionError::INVALID_PSBT: - return "PSBT is not sane"; + return Untranslated("PSBT is not sane"); case TransactionError::PSBT_MISMATCH: - return "PSBTs not compatible (different transactions)"; + return Untranslated("PSBTs not compatible (different transactions)"); case TransactionError::SIGHASH_MISMATCH: - return "Specified sighash value does not match existing value"; + return Untranslated("Specified sighash value does not match existing value"); case TransactionError::MAX_FEE_EXCEEDED: - return "Fee exceeds maximum configured by -maxtxfee"; + return Untranslated("Fee exceeds maximum configured by -maxtxfee"); // no default case, so the compiler can warn about missing cases } assert(false); } -std::string ResolveErrMsg(const std::string& optname, const std::string& strBind) +bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind) { - return strprintf(_("Cannot resolve -%s address: '%s'").translated, optname, strBind); + return strprintf(_("Cannot resolve -%s address: '%s'"), optname, strBind); } bilingual_str AmountHighWarn(const std::string& optname) diff --git a/src/util/error.h b/src/util/error.h index 61af88ddea..b9830c9eea 100644 --- a/src/util/error.h +++ b/src/util/error.h @@ -32,9 +32,9 @@ enum class TransactionError { MAX_FEE_EXCEEDED, }; -std::string TransactionErrorString(const TransactionError error); +bilingual_str TransactionErrorString(const TransactionError error); -std::string ResolveErrMsg(const std::string& optname, const std::string& strBind); +bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind); bilingual_str AmountHighWarn(const std::string& optname); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 89737ca7b5..a59aa8b980 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2997,7 +2997,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac } if (nFeeRet > m_default_max_tx_fee) { - error = Untranslated(TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)); + error = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED); return false; } |