diff options
author | Ryan Ofsky <ryan@ofsky.org> | 2024-12-04 10:49:13 -0500 |
---|---|---|
committer | Ryan Ofsky <ryan@ofsky.org> | 2024-12-04 11:15:58 -0500 |
commit | 39950e148d80eec7ef18ff2858453d34a86c15cb (patch) | |
tree | e2c6edfd51c50483588d0d9f34c112415d617750 | |
parent | ae69fc37e4fff237a119225061d68f69e6cd61d7 (diff) | |
parent | fa3e074304780549b1e7972217930e34fa55f59a (diff) |
Merge bitcoin/bitcoin#31295: refactor: Prepare compile-time check of bilingual format strings
fa3e074304780549b1e7972217930e34fa55f59a refactor: Tidy fixups (MarcoFalke)
fa72646f2b197810a324cb0544d9a1fac37d3f9c move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0aac3b5ec26d3c7fc98240f879f9906 refactor: Pick translated string after format (MarcoFalke)
Pull request description:
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304780549b1e7972217930e34fa55f59a
hodlinator:
cr-ACK fa3e074304780549b1e7972217930e34fa55f59a
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
-rw-r--r-- | src/clientversion.cpp | 12 | ||||
-rw-r--r-- | src/test/util_string_tests.cpp | 6 | ||||
-rw-r--r-- | src/util/strencodings.h | 37 | ||||
-rw-r--r-- | src/util/string.h | 124 | ||||
-rw-r--r-- | src/util/translation.h | 6 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 24 | ||||
-rw-r--r-- | src/wallet/salvage.cpp | 6 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 4 | ||||
-rwxr-xr-x | test/lint/run-lint-format-strings.py | 2 |
9 files changed, 113 insertions, 108 deletions
diff --git a/src/clientversion.cpp b/src/clientversion.cpp index 6fa3c700ee..af58c3023f 100644 --- a/src/clientversion.cpp +++ b/src/clientversion.cpp @@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const std::string CopyrightHolders(const std::string& strPrefix) { - const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION); + const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated; std::string strCopyrightHolders = strPrefix + copyright_devs; // Make sure Bitcoin Core copyright is not removed by accident @@ -85,15 +85,17 @@ std::string LicenseInfo() { const std::string URL_SOURCE_CODE = "<https://github.com/bitcoin/bitcoin>"; - return CopyrightHolders(strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR) + " ") + "\n" + + return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated + " ") + "\n" + "\n" + strprintf(_("Please contribute if you find %s useful. " - "Visit %s for further information about the software.").translated, CLIENT_NAME, "<" CLIENT_URL ">") + + "Visit %s for further information about the software."), + CLIENT_NAME, "<" CLIENT_URL ">") + .translated + "\n" + - strprintf(_("The source code is available from %s.").translated, URL_SOURCE_CODE) + + strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated + "\n" + "\n" + _("This is experimental software.").translated + "\n" + - strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s").translated, "COPYING", "<https://opensource.org/licenses/MIT>") + + strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated + "\n"; } diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp index 9f5b702acd..fbab9e7aba 100644 --- a/src/test/util_string_tests.cpp +++ b/src/test/util_string_tests.cpp @@ -16,13 +16,13 @@ BOOST_AUTO_TEST_SUITE(util_string_tests) template <unsigned NumArgs> inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt) { - // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused. - decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt); + // Execute compile-time check again at run-time to get code coverage stats + util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt); } template <unsigned WrongNumArgs> inline void FailFmtWithError(const char* wrong_fmt, std::string_view error) { - BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error)); + BOOST_CHECK_EXCEPTION(util::detail::CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*, HasReason{error}); } BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec) diff --git a/src/util/strencodings.h b/src/util/strencodings.h index 1543de03ab..d0809162fa 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -377,6 +377,24 @@ consteval uint8_t ConstevalHexDigit(const char c) throw "Only lowercase hex digits are allowed, for consistency"; } +namespace detail { +template <size_t N> +struct Hex { + std::array<std::byte, N / 2> bytes{}; + consteval Hex(const char (&hex_str)[N]) + // 2 hex digits required per byte + implicit null terminator + requires(N % 2 == 1) + { + if (hex_str[N - 1]) throw "null terminator required"; + for (std::size_t i = 0; i < bytes.size(); ++i) { + bytes[i] = static_cast<std::byte>( + (ConstevalHexDigit(hex_str[2 * i]) << 4) | + ConstevalHexDigit(hex_str[2 * i + 1])); + } + } +}; +} // namespace detail + /** * ""_hex is a compile-time user-defined literal returning a * `std::array<std::byte>`, equivalent to ParseHex(). Variants provided: @@ -407,25 +425,6 @@ consteval uint8_t ConstevalHexDigit(const char c) * time/runtime barrier. */ inline namespace hex_literals { -namespace detail { - -template <size_t N> -struct Hex { - std::array<std::byte, N / 2> bytes{}; - consteval Hex(const char (&hex_str)[N]) - // 2 hex digits required per byte + implicit null terminator - requires(N % 2 == 1) - { - if (hex_str[N - 1]) throw "null terminator required"; - for (std::size_t i = 0; i < bytes.size(); ++i) { - bytes[i] = static_cast<std::byte>( - (ConstevalHexDigit(hex_str[2 * i]) << 4) | - ConstevalHexDigit(hex_str[2 * i + 1])); - } - } -}; - -} // namespace detail template <util::detail::Hex str> constexpr auto operator""_hex() { return str.bytes; } diff --git a/src/util/string.h b/src/util/string.h index c9e33e6592..b523e6ef4e 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -17,6 +17,69 @@ #include <vector> namespace util { +namespace detail { +template <unsigned num_params> +constexpr static void CheckNumFormatSpecifiers(const char* str) +{ + unsigned count_normal{0}; // Number of "normal" specifiers, like %s + unsigned count_pos{0}; // Max number in positional specifier, like %8$s + for (auto it{str}; *it != '\0'; ++it) { + if (*it != '%' || *++it == '%') continue; // Skip escaped %% + + auto add_arg = [&] { + unsigned maybe_num{0}; + while ('0' <= *it && *it <= '9') { + maybe_num *= 10; + maybe_num += *it - '0'; + ++it; + } + + if (*it == '$') { + ++it; + // Positional specifier, like %8$s + if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; + count_pos = std::max(count_pos, maybe_num); + } else { + // Non-positional specifier, like %s + ++count_normal; + } + }; + + // Increase argument count and consume positional specifier, if present. + add_arg(); + + // Consume flags. + while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it; + + auto parse_size = [&] { + if (*it == '*') { + ++it; + add_arg(); + } else { + while ('0' <= *it && *it <= '9') ++it; + } + }; + + // Consume dynamic or static width value. + parse_size(); + + // Consume dynamic or static precision value. + if (*it == '.') { + ++it; + parse_size(); + } + + if (*it == '\0') throw "Format specifier incorrectly terminated by end of string"; + + // Length and type in "[flags][width][.precision][length]type" + // is not checked. Parsing continues with the next '%'. + } + if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!"; + unsigned count{count_normal | count_pos}; + if (num_params != count) throw "Format specifier count must match the argument count!"; +} +} // namespace detail + /** * @brief A wrapper for a compile-time partially validated format string * @@ -28,66 +91,7 @@ namespace util { template <unsigned num_params> struct ConstevalFormatString { const char* const fmt; - consteval ConstevalFormatString(const char* str) : fmt{str} { Detail_CheckNumFormatSpecifiers(fmt); } - constexpr static void Detail_CheckNumFormatSpecifiers(const char* str) - { - unsigned count_normal{0}; // Number of "normal" specifiers, like %s - unsigned count_pos{0}; // Max number in positional specifier, like %8$s - for (auto it{str}; *it != '\0'; ++it) { - if (*it != '%' || *++it == '%') continue; // Skip escaped %% - - auto add_arg = [&] { - unsigned maybe_num{0}; - while ('0' <= *it && *it <= '9') { - maybe_num *= 10; - maybe_num += *it - '0'; - ++it; - } - - if (*it == '$') { - ++it; - // Positional specifier, like %8$s - if (maybe_num == 0) throw "Positional format specifier must have position of at least 1"; - count_pos = std::max(count_pos, maybe_num); - } else { - // Non-positional specifier, like %s - ++count_normal; - } - }; - - // Increase argument count and consume positional specifier, if present. - add_arg(); - - // Consume flags. - while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it; - - auto parse_size = [&] { - if (*it == '*') { - ++it; - add_arg(); - } else { - while ('0' <= *it && *it <= '9') ++it; - } - }; - - // Consume dynamic or static width value. - parse_size(); - - // Consume dynamic or static precision value. - if (*it == '.') { - ++it; - parse_size(); - } - - if (*it == '\0') throw "Format specifier incorrectly terminated by end of string"; - - // Length and type in "[flags][width][.precision][length]type" - // is not checked. Parsing continues with the next '%'. - } - if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!"; - unsigned count{count_normal | count_pos}; - if (num_params != count) throw "Format specifier count must match the argument count!"; - } + consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers<num_params>(fmt); } }; void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute); diff --git a/src/util/translation.h b/src/util/translation.h index 6effe102f9..7c734a1766 100644 --- a/src/util/translation.h +++ b/src/util/translation.h @@ -10,6 +10,9 @@ #include <functional> #include <string> +/** Translate a message to the native language of the user. */ +const extern std::function<std::string(const char*)> G_TRANSLATION_FUN; + /** * Bilingual messages: * - in GUI: user's native language + untranslated (i.e. English) @@ -64,9 +67,6 @@ bilingual_str format(const bilingual_str& fmt, const Args&... args) } } // namespace tinyformat -/** Translate a message to the native language of the user. */ -const extern std::function<std::string(const char*)> G_TRANSLATION_FUN; - struct ConstevalStringLiteral { const char* const lit; consteval ConstevalStringLiteral(const char* str) : lit{str} {} diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 3184d0f3b0..68ad58d284 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -24,24 +24,24 @@ namespace wallet { static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { if (wallet.HasWalletSpend(wtx.tx)) { - errors.push_back(Untranslated("Transaction has descendants in the wallet")); + errors.emplace_back(Untranslated("Transaction has descendants in the wallet")); return feebumper::Result::INVALID_PARAMETER; } { if (wallet.chain().hasDescendantsInMempool(wtx.GetHash())) { - errors.push_back(Untranslated("Transaction has descendants in the mempool")); + errors.emplace_back(Untranslated("Transaction has descendants in the mempool")); return feebumper::Result::INVALID_PARAMETER; } } if (wallet.GetTxDepthInMainChain(wtx) != 0) { - errors.push_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction")); + errors.emplace_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction")); return feebumper::Result::WALLET_ERROR; } if (!SignalsOptInRBF(*wtx.tx)) { - errors.push_back(Untranslated("Transaction is not BIP 125 replaceable")); + errors.emplace_back(Untranslated("Transaction is not BIP 125 replaceable")); return feebumper::Result::WALLET_ERROR; } @@ -55,7 +55,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee) isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE; if (!AllInputsMine(wallet, *wtx.tx, filter)) { - errors.push_back(Untranslated("Transaction contains inputs that don't belong to this wallet")); + errors.emplace_back(Untranslated("Transaction contains inputs that don't belong to this wallet")); return feebumper::Result::WALLET_ERROR; } } @@ -167,7 +167,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo { // For now, cannot specify both new outputs to use and an output index to send change if (!outputs.empty() && original_change_index.has_value()) { - errors.push_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.")); + errors.emplace_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.")); return Result::INVALID_PARAMETER; } @@ -178,14 +178,14 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo errors.clear(); auto it = wallet.mapWallet.find(txid); if (it == wallet.mapWallet.end()) { - errors.push_back(Untranslated("Invalid or non-wallet transaction id")); + errors.emplace_back(Untranslated("Invalid or non-wallet transaction id")); return Result::INVALID_ADDRESS_OR_KEY; } const CWalletTx& wtx = it->second; // Make sure that original_change_index is valid if (original_change_index.has_value() && original_change_index.value() >= wtx.tx->vout.size()) { - errors.push_back(Untranslated("Change position is out of range")); + errors.emplace_back(Untranslated("Change position is out of range")); return Result::INVALID_PARAMETER; } @@ -201,7 +201,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo for (const CTxIn& txin : wtx.tx->vin) { const Coin& coin = coins.at(txin.prevout); if (coin.out.IsNull()) { - errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); + errors.emplace_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); return Result::MISC_ERROR; } PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout); @@ -319,7 +319,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, new_coin_control, false); if (!res) { - errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); + errors.emplace_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); return Result::WALLET_ERROR; } @@ -361,7 +361,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti } auto it = txid.IsNull() ? wallet.mapWallet.end() : wallet.mapWallet.find(txid); if (it == wallet.mapWallet.end()) { - errors.push_back(Untranslated("Invalid or non-wallet transaction id")); + errors.emplace_back(Untranslated("Invalid or non-wallet transaction id")); return Result::MISC_ERROR; } const CWalletTx& oldWtx = it->second; @@ -382,7 +382,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti // mark the original tx as bumped bumped_txid = tx->GetHash(); if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) { - errors.push_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced")); + errors.emplace_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced")); } return Result::OK; } diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 0ac1b66897..457119396a 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -117,7 +117,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil Db db(env->dbenv.get(), 0); result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE); if (result == DB_VERIFY_BAD) { - warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); + warnings.emplace_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable.")); } if (result != 0 && result != DB_VERIFY_BAD) { error = strprintf(Untranslated("Salvage: Database salvage failed with result %d."), result); @@ -144,7 +144,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil break; getline(strDump, valueHex); if (valueHex == DATA_END) { - warnings.push_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); + warnings.emplace_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values.")); break; } salvagedData.emplace_back(ParseHex(keyHex), ParseHex(valueHex)); @@ -153,7 +153,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil bool fSuccess; if (keyHex != DATA_END) { - warnings.push_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")); + warnings.emplace_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output.")); fSuccess = false; } else { fSuccess = (result == 0); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 82c0d2caf1..c0341a8111 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -290,7 +290,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s // Legacy wallets are being deprecated, warn if the loaded wallet is legacy if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); + warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); } NotifyWalletLoaded(context, wallet); @@ -483,7 +483,7 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& // Legacy wallets are being deprecated, warn if a newly created wallet is legacy if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) { - warnings.push_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); + warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future.")); } status = DatabaseStatus::SUCCESS; diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index d3c0ac92e5..4402ec2d57 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -13,7 +13,7 @@ import re import sys FALSE_POSITIVES = [ - ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), + ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), ] |