Merge bitcoin/bitcoin#31295: refactor: Prepare compile-time check of bilingual format strings

fa3e074304 refactor: Tidy fixups (MarcoFalke)
fa72646f2b move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0 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 fa3e074304. Nice changes! These should allow related PRs to be simpler.
  l0rinc:
    ACK fa3e074304
  hodlinator:
    cr-ACK fa3e074304

Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a

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";
 }

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.
+    // Execute compile-time check again at run-time to get code coverage stats
-    decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
+    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)

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; }

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); }
+    consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers<num_params>(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!";
-    }
 };
 
 void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute);

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} {}

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;
 }

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);

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;

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)"),
 ]