Skip to content

Commit 39950e1

Browse files
ryanofskyryanofsky
committed
Merge bitcoin/bitcoin#31295: refactor: Prepare compile-time check of bilingual format strings
fa3e074 refactor: Tidy fixups (MarcoFalke) fa72646 move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke) faff840 refactor: Pick translated string after format (MarcoFalke) Pull request description: The changes are required for bitcoin/bitcoin#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 fa3e074. Nice changes! These should allow related PRs to be simpler. l0rinc: ACK fa3e074 hodlinator: cr-ACK fa3e074 Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
2 parents ae69fc3 + fa3e074 commit 39950e1

File tree

9 files changed

+113
-108
lines changed

9 files changed

+113
-108
lines changed

‎src/clientversion.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const
7171

7272
std::string CopyrightHolders(const std::string& strPrefix)
7373
{
74-
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION);
74+
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;
7575
std::string strCopyrightHolders = strPrefix + copyright_devs;
7676

7777
// Make sure Bitcoin Core copyright is not removed by accident
@@ -85,15 +85,17 @@ std::string LicenseInfo()
8585
{
8686
const std::string URL_SOURCE_CODE = "<https://github.com/bitcoin/bitcoin>";
8787

88-
return CopyrightHolders(strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR) + " ") + "\n" +
88+
return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated + " ") + "\n" +
8989
"\n" +
9090
strprintf(_("Please contribute if you find %s useful. "
91-
"Visit %s for further information about the software.").translated, CLIENT_NAME, "<" CLIENT_URL ">") +
91+
"Visit %s for further information about the software."),
92+
CLIENT_NAME, "<" CLIENT_URL ">")
93+
.translated +
9294
"\n" +
93-
strprintf(_("The source code is available from %s.").translated, URL_SOURCE_CODE) +
95+
strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated +
9496
"\n" +
9597
"\n" +
9698
_("This is experimental software.").translated + "\n" +
97-
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s").translated, "COPYING", "<https://opensource.org/licenses/MIT>") +
99+
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated +
98100
"\n";
99101
}

‎src/test/util_string_tests.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@ BOOST_AUTO_TEST_SUITE(util_string_tests)
1616
template <unsigned NumArgs>
1717
inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
1818
{
19-
// This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20-
decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
19+
// Execute compile-time check again at run-time to get code coverage stats
20+
util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
2121
}
2222
template <unsigned WrongNumArgs>
2323
inline void FailFmtWithError(const char* wrong_fmt, std::string_view error)
2424
{
25-
BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason(error));
25+
BOOST_CHECK_EXCEPTION(util::detail::CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*, HasReason{error});
2626
}
2727

2828
BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)

‎src/util/strencodings.h‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,24 @@ consteval uint8_t ConstevalHexDigit(const char c)
377377
throw "Only lowercase hex digits are allowed, for consistency";
378378
}
379379

380+
namespace detail {
381+
template <size_t N>
382+
struct Hex {
383+
std::array<std::byte, N / 2> bytes{};
384+
consteval Hex(const char (&hex_str)[N])
385+
// 2 hex digits required per byte + implicit null terminator
386+
requires(N % 2 == 1)
387+
{
388+
if (hex_str[N - 1]) throw "null terminator required";
389+
for (std::size_t i = 0; i < bytes.size(); ++i) {
390+
bytes[i] = static_cast<std::byte>(
391+
(ConstevalHexDigit(hex_str[2 * i]) << 4) |
392+
ConstevalHexDigit(hex_str[2 * i + 1]));
393+
}
394+
}
395+
};
396+
} // namespace detail
397+
380398
/**
381399
* ""_hex is a compile-time user-defined literal returning a
382400
* `std::array<std::byte>`, equivalent to ParseHex(). Variants provided:
@@ -407,25 +425,6 @@ consteval uint8_t ConstevalHexDigit(const char c)
407425
* time/runtime barrier.
408426
*/
409427
inline namespace hex_literals {
410-
namespace detail {
411-
412-
template <size_t N>
413-
struct Hex {
414-
std::array<std::byte, N / 2> bytes{};
415-
consteval Hex(const char (&hex_str)[N])
416-
// 2 hex digits required per byte + implicit null terminator
417-
requires(N % 2 == 1)
418-
{
419-
if (hex_str[N - 1]) throw "null terminator required";
420-
for (std::size_t i = 0; i < bytes.size(); ++i) {
421-
bytes[i] = static_cast<std::byte>(
422-
(ConstevalHexDigit(hex_str[2 * i]) << 4) |
423-
ConstevalHexDigit(hex_str[2 * i + 1]));
424-
}
425-
}
426-
};
427-
428-
} // namespace detail
429428

430429
template <util::detail::Hex str>
431430
constexpr auto operator""_hex() { return str.bytes; }

‎src/util/string.h‎

Lines changed: 64 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,69 @@
1717
#include <vector>
1818

1919
namespace util {
20+
namespace detail {
21+
template <unsigned num_params>
22+
constexpr static void CheckNumFormatSpecifiers(const char* str)
23+
{
24+
unsigned count_normal{0}; // Number of "normal" specifiers, like %s
25+
unsigned count_pos{0}; // Max number in positional specifier, like %8$s
26+
for (auto it{str}; *it != '\0'; ++it) {
27+
if (*it != '%' || *++it == '%') continue; // Skip escaped %%
28+
29+
auto add_arg = [&] {
30+
unsigned maybe_num{0};
31+
while ('0' <= *it && *it <= '9') {
32+
maybe_num *= 10;
33+
maybe_num += *it - '0';
34+
++it;
35+
}
36+
37+
if (*it == '$') {
38+
++it;
39+
// Positional specifier, like %8$s
40+
if (maybe_num == 0) throw "Positional format specifier must have position of at least 1";
41+
count_pos = std::max(count_pos, maybe_num);
42+
} else {
43+
// Non-positional specifier, like %s
44+
++count_normal;
45+
}
46+
};
47+
48+
// Increase argument count and consume positional specifier, if present.
49+
add_arg();
50+
51+
// Consume flags.
52+
while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it;
53+
54+
auto parse_size = [&] {
55+
if (*it == '*') {
56+
++it;
57+
add_arg();
58+
} else {
59+
while ('0' <= *it && *it <= '9') ++it;
60+
}
61+
};
62+
63+
// Consume dynamic or static width value.
64+
parse_size();
65+
66+
// Consume dynamic or static precision value.
67+
if (*it == '.') {
68+
++it;
69+
parse_size();
70+
}
71+
72+
if (*it == '\0') throw "Format specifier incorrectly terminated by end of string";
73+
74+
// Length and type in "[flags][width][.precision][length]type"
75+
// is not checked. Parsing continues with the next '%'.
76+
}
77+
if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!";
78+
unsigned count{count_normal | count_pos};
79+
if (num_params != count) throw "Format specifier count must match the argument count!";
80+
}
81+
} // namespace detail
82+
2083
/**
2184
* @brief A wrapper for a compile-time partially validated format string
2285
*
@@ -28,66 +91,7 @@ namespace util {
2891
template <unsigned num_params>
2992
struct ConstevalFormatString {
3093
const char* const fmt;
31-
consteval ConstevalFormatString(const char* str) : fmt{str} { Detail_CheckNumFormatSpecifiers(fmt); }
32-
constexpr static void Detail_CheckNumFormatSpecifiers(const char* str)
33-
{
34-
unsigned count_normal{0}; // Number of "normal" specifiers, like %s
35-
unsigned count_pos{0}; // Max number in positional specifier, like %8$s
36-
for (auto it{str}; *it != '\0'; ++it) {
37-
if (*it != '%' || *++it == '%') continue; // Skip escaped %%
38-
39-
auto add_arg = [&] {
40-
unsigned maybe_num{0};
41-
while ('0' <= *it && *it <= '9') {
42-
maybe_num *= 10;
43-
maybe_num += *it - '0';
44-
++it;
45-
}
46-
47-
if (*it == '$') {
48-
++it;
49-
// Positional specifier, like %8$s
50-
if (maybe_num == 0) throw "Positional format specifier must have position of at least 1";
51-
count_pos = std::max(count_pos, maybe_num);
52-
} else {
53-
// Non-positional specifier, like %s
54-
++count_normal;
55-
}
56-
};
57-
58-
// Increase argument count and consume positional specifier, if present.
59-
add_arg();
60-
61-
// Consume flags.
62-
while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it;
63-
64-
auto parse_size = [&] {
65-
if (*it == '*') {
66-
++it;
67-
add_arg();
68-
} else {
69-
while ('0' <= *it && *it <= '9') ++it;
70-
}
71-
};
72-
73-
// Consume dynamic or static width value.
74-
parse_size();
75-
76-
// Consume dynamic or static precision value.
77-
if (*it == '.') {
78-
++it;
79-
parse_size();
80-
}
81-
82-
if (*it == '\0') throw "Format specifier incorrectly terminated by end of string";
83-
84-
// Length and type in "[flags][width][.precision][length]type"
85-
// is not checked. Parsing continues with the next '%'.
86-
}
87-
if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!";
88-
unsigned count{count_normal | count_pos};
89-
if (num_params != count) throw "Format specifier count must match the argument count!";
90-
}
94+
consteval ConstevalFormatString(const char* str) : fmt{str} { detail::CheckNumFormatSpecifiers<num_params>(fmt); }
9195
};
9296

9397
void ReplaceAll(std::string& in_out, const std::string& search, const std::string& substitute);

‎src/util/translation.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
#include <functional>
1111
#include <string>
1212

13+
/** Translate a message to the native language of the user. */
14+
const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
15+
1316
/**
1417
* Bilingual messages:
1518
* - in GUI: user's native language + untranslated (i.e. English)
@@ -64,9 +67,6 @@ bilingual_str format(const bilingual_str& fmt, const Args&... args)
6467
}
6568
} // namespace tinyformat
6669

67-
/** Translate a message to the native language of the user. */
68-
const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
69-
7070
struct ConstevalStringLiteral {
7171
const char* const lit;
7272
consteval ConstevalStringLiteral(const char* str) : lit{str} {}

‎src/wallet/feebumper.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,24 +24,24 @@ namespace wallet {
2424
static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
2525
{
2626
if (wallet.HasWalletSpend(wtx.tx)) {
27-
errors.push_back(Untranslated("Transaction has descendants in the wallet"));
27+
errors.emplace_back(Untranslated("Transaction has descendants in the wallet"));
2828
return feebumper::Result::INVALID_PARAMETER;
2929
}
3030

3131
{
3232
if (wallet.chain().hasDescendantsInMempool(wtx.GetHash())) {
33-
errors.push_back(Untranslated("Transaction has descendants in the mempool"));
33+
errors.emplace_back(Untranslated("Transaction has descendants in the mempool"));
3434
return feebumper::Result::INVALID_PARAMETER;
3535
}
3636
}
3737

3838
if (wallet.GetTxDepthInMainChain(wtx) != 0) {
39-
errors.push_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction"));
39+
errors.emplace_back(Untranslated("Transaction has been mined, or is conflicted with a mined transaction"));
4040
return feebumper::Result::WALLET_ERROR;
4141
}
4242

4343
if (!SignalsOptInRBF(*wtx.tx)) {
44-
errors.push_back(Untranslated("Transaction is not BIP 125 replaceable"));
44+
errors.emplace_back(Untranslated("Transaction is not BIP 125 replaceable"));
4545
return feebumper::Result::WALLET_ERROR;
4646
}
4747

@@ -55,7 +55,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
5555
// 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)
5656
isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
5757
if (!AllInputsMine(wallet, *wtx.tx, filter)) {
58-
errors.push_back(Untranslated("Transaction contains inputs that don't belong to this wallet"));
58+
errors.emplace_back(Untranslated("Transaction contains inputs that don't belong to this wallet"));
5959
return feebumper::Result::WALLET_ERROR;
6060
}
6161
}
@@ -167,7 +167,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
167167
{
168168
// For now, cannot specify both new outputs to use and an output index to send change
169169
if (!outputs.empty() && original_change_index.has_value()) {
170-
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."));
170+
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."));
171171
return Result::INVALID_PARAMETER;
172172
}
173173

@@ -178,14 +178,14 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
178178
errors.clear();
179179
auto it = wallet.mapWallet.find(txid);
180180
if (it == wallet.mapWallet.end()) {
181-
errors.push_back(Untranslated("Invalid or non-wallet transaction id"));
181+
errors.emplace_back(Untranslated("Invalid or non-wallet transaction id"));
182182
return Result::INVALID_ADDRESS_OR_KEY;
183183
}
184184
const CWalletTx& wtx = it->second;
185185

186186
// Make sure that original_change_index is valid
187187
if (original_change_index.has_value() && original_change_index.value() >= wtx.tx->vout.size()) {
188-
errors.push_back(Untranslated("Change position is out of range"));
188+
errors.emplace_back(Untranslated("Change position is out of range"));
189189
return Result::INVALID_PARAMETER;
190190
}
191191

@@ -201,7 +201,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
201201
for (const CTxIn& txin : wtx.tx->vin) {
202202
const Coin& coin = coins.at(txin.prevout);
203203
if (coin.out.IsNull()) {
204-
errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n)));
204+
errors.emplace_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n)));
205205
return Result::MISC_ERROR;
206206
}
207207
PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout);
@@ -319,7 +319,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
319319

320320
auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, new_coin_control, false);
321321
if (!res) {
322-
errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
322+
errors.emplace_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
323323
return Result::WALLET_ERROR;
324324
}
325325

@@ -361,7 +361,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti
361361
}
362362
auto it = txid.IsNull() ? wallet.mapWallet.end() : wallet.mapWallet.find(txid);
363363
if (it == wallet.mapWallet.end()) {
364-
errors.push_back(Untranslated("Invalid or non-wallet transaction id"));
364+
errors.emplace_back(Untranslated("Invalid or non-wallet transaction id"));
365365
return Result::MISC_ERROR;
366366
}
367367
const CWalletTx& oldWtx = it->second;
@@ -382,7 +382,7 @@ Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransacti
382382
// mark the original tx as bumped
383383
bumped_txid = tx->GetHash();
384384
if (!wallet.MarkReplaced(oldWtx.GetHash(), bumped_txid)) {
385-
errors.push_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced"));
385+
errors.emplace_back(Untranslated("Created new bumpfee transaction but could not mark the original transaction as replaced"));
386386
}
387387
return Result::OK;
388388
}

‎src/wallet/salvage.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
117117
Db db(env->dbenv.get(), 0);
118118
result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE);
119119
if (result == DB_VERIFY_BAD) {
120-
warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
120+
warnings.emplace_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
121121
}
122122
if (result != 0 && result != DB_VERIFY_BAD) {
123123
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
144144
break;
145145
getline(strDump, valueHex);
146146
if (valueHex == DATA_END) {
147-
warnings.push_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values."));
147+
warnings.emplace_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values."));
148148
break;
149149
}
150150
salvagedData.emplace_back(ParseHex(keyHex), ParseHex(valueHex));
@@ -153,7 +153,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
153153

154154
bool fSuccess;
155155
if (keyHex != DATA_END) {
156-
warnings.push_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output."));
156+
warnings.emplace_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output."));
157157
fSuccess = false;
158158
} else {
159159
fSuccess = (result == 0);

0 commit comments

Comments
 (0)