Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Clean up messy strformat and bilingual_str usages #31072

Merged
merged 4 commits into from
Dec 6, 2024
Merged

refactor: Clean up messy strformat and bilingual_str usages #31072

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0580219
refactor: Avoid concatenation of format strings
ryanofsky Oct 11, 2024
831d2bf
refactor: Don't embed translated string in untranslated string.
ryanofsky Oct 9, 2024
006e4d1
refactor: Use + instead of strformat to concatenate translated & untr…
ryanofsky Oct 12, 2024
0184d33
scripted-diff: Replace strprintf(Untranslated) with Untranslated(strp…
ryanofsky Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
scripted-diff: Replace strprintf(Untranslated) with Untranslated(strp… 
…rintf)

This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.

-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
  • Loading branch information
@ryanofsky
ryanofsky committed Dec 4, 2024
commit 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5
2 changes: 1 addition & 1 deletion src/httpserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ static bool InitHTTPAllowList()
const CSubNet subnet{LookupSubNet(strAllow)};
if (!subnet.IsValid()) {
uiInterface.ThreadSafeMessageBox(
strprintf(Untranslated("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24)."), strAllow),
Untranslated(strprintf("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24).", strAllow)),
"", CClientUIInterface::MSG_ERROR);
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/index/base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ bool BaseIndex::Init()
// best chain, we will rewind to the fork point during index sync
const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};
if (!locator_index) {
return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName()));
return InitError(Untranslated(strprintf("%s: best block of the index not found. Please rebuild the index.", GetName())));
}
SetBestBlockIndex(locator_index);
}
Expand Down
6 changes: 3 additions & 3 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
try {
node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown_signal), chainman_opts, blockman_opts);
} catch (std::exception& e) {
return {ChainstateLoadStatus::FAILURE_FATAL, strprintf(Untranslated("Failed to initialize ChainstateManager: %s"), e.what())};
return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated(strprintf("Failed to initialize ChainstateManager: %s", e.what()))};
}
ChainstateManager& chainman = *node.chainman;
// This is defined and set here instead of inline in validation.h to avoid a hard
Expand Down Expand Up @@ -1339,7 +1339,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
try {
ipc->listenAddress(address);
} catch (const std::exception& e) {
return InitError(strprintf(Untranslated("Unable to bind to IPC address '%s'. %s"), address, e.what()));
return InitError(Untranslated(strprintf("Unable to bind to IPC address '%s'. %s", address, e.what())));
}
LogPrintf("Listening for IPC requests on address %s\n", address);
}
Expand Down Expand Up @@ -2044,7 +2044,7 @@ bool StartIndexBackgroundSync(NodeContext& node)
const CBlockIndex* start_block = *indexes_start_block;
if (!start_block) start_block = chainman.ActiveChain().Genesis();
if (!chainman.m_blockman.CheckBlockDataAvailability(*index_chain.Tip(), *Assert(start_block))) {
return InitError(strprintf(Untranslated("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"), older_index_name));
return InitError(Untranslated(strprintf("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)", older_index_name)));
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/init/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ bool StartLogging(const ArgsManager& args)
}
}
if (!LogInstance().StartLogging()) {
return InitError(strprintf(Untranslated("Could not open debug log file %s"),
fs::PathToString(LogInstance().m_file_path)));
return InitError(Untranslated(strprintf("Could not open debug log file %s",
fs::PathToString(LogInstance().m_file_path))));
}

if (!LogInstance().m_log_timestamps)
Expand Down
10 changes: 5 additions & 5 deletions src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3058,22 +3058,22 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
socklen_t len = sizeof(sockaddr);
if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len))
{
strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToStringAddrPort());
strError = Untranslated(strprintf("Bind address family for %s not supported", addrBind.ToStringAddrPort()));
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original);
return false;
}

std::unique_ptr<Sock> sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP);
if (!sock) {
strError = strprintf(Untranslated("Couldn't open socket for incoming connections (socket returned error %s)"), NetworkErrorString(WSAGetLastError()));
strError = Untranslated(strprintf("Couldn't open socket for incoming connections (socket returned error %s)", NetworkErrorString(WSAGetLastError())));
LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original);
return false;
}

// Allow binding if the port is still in TIME_WAIT state after
// the program was closed and restarted.
if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
strError = strprintf(Untranslated("Error setting SO_REUSEADDR on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
strError = Untranslated(strprintf("Error setting SO_REUSEADDR on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError())));
LogPrintf("%s\n", strError.original);
}

Expand All @@ -3082,14 +3082,14 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
if (addrBind.IsIPv6()) {
#ifdef IPV6_V6ONLY
if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
strError = strprintf(Untranslated("Error setting IPV6_V6ONLY on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
strError = Untranslated(strprintf("Error setting IPV6_V6ONLY on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError())));
LogPrintf("%s\n", strError.original);
}
#endif
#ifdef WIN32
int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED;
if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) {
strError = strprintf(Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError()));
strError = Untranslated(strprintf("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway", NetworkErrorString(WSAGetLastError())));
LogPrintf("%s\n", strError.original);
}
#endif
Expand Down
4 changes: 2 additions & 2 deletions src/node/chainstatemanager_args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
if (auto min_work{uint256::FromUserHex(*value)}) {
opts.minimum_chain_work = UintToArith256(*min_work);
} else {
return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d hex digits"), *value, uint256::size() * 2)};
return util::Error{Untranslated(strprintf("Invalid minimum work specified (%s), must be up to %d hex digits", *value, uint256::size() * 2))};
}
}

if (auto value{args.GetArg("-assumevalid")}) {
if (auto block_hash{uint256::FromUserHex(*value)}) {
opts.assumed_valid_block = *block_hash;
} else {
return util::Error{strprintf(Untranslated("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)"), *value, uint256::size() * 2)};
return util::Error{Untranslated(strprintf("Invalid assumevalid block hash specified (%s), must be up to %d hex digits (or 0 to disable)", *value, uint256::size() * 2))};
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/node/mempool_args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP

mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", DEFAULT_ACCEPT_NON_STD_TXN);
if (!chainparams.IsTestChain() && !mempool_opts.require_standard) {
return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.GetChainTypeString())};
return util::Error{Untranslated(strprintf("acceptnonstdtxn is not currently supported for %s chain", chainparams.GetChainTypeString()))};
}

mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat);
Expand Down
2 changes: 1 addition & 1 deletion src/qt/bitcoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ int GuiMain(int argc, char* argv[])
SetupUIArgs(gArgs);
std::string error;
if (!gArgs.ParseParameters(argc, argv, error)) {
InitError(strprintf(Untranslated("Error parsing command line arguments: %s"), error));
InitError(Untranslated(strprintf("Error parsing command line arguments: %s", error)));
// Create a message box, because the gui has neither been created nor has subscribed to core signals
QMessageBox::critical(nullptr, CLIENT_NAME,
// message cannot be translated because translations have not been initialized
Expand Down
2 changes: 1 addition & 1 deletion src/test/result_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ util::Result<int> IntFn(int i, bool success)
util::Result<bilingual_str> StrFn(bilingual_str s, bool success)
{
if (success) return s;
return util::Error{strprintf(Untranslated("str %s error."), s.original)};
return util::Error{Untranslated(strprintf("str %s error.", s.original))};
}

util::Result<NoCopy> NoCopyFn(int i, bool success)
Expand Down
38 changes: 19 additions & 19 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5719,20 +5719,20 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) {
auto available_heights = GetParams().GetAvailableSnapshotHeights();
std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); });
return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s"),
return util::Error{Untranslated(strprintf("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s",
base_blockhash.ToString(),
heights_formatted)};
heights_formatted))};
}

snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash);
if (!snapshot_start_block) {
return util::Error{strprintf(Untranslated("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again"),
base_blockhash.ToString())};
return util::Error{Untranslated(strprintf("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again",
base_blockhash.ToString()))};
}

bool start_block_invalid = snapshot_start_block->nStatus & BLOCK_FAILED_MASK;
if (start_block_invalid) {
return util::Error{strprintf(Untranslated("The base block header (%s) is part of an invalid chain"), base_blockhash.ToString())};
return util::Error{Untranslated(strprintf("The base block header (%s) is part of an invalid chain", base_blockhash.ToString()))};
}

if (!m_best_header || m_best_header->GetAncestor(snapshot_start_block->nHeight) != snapshot_start_block) {
Expand Down Expand Up @@ -5892,16 +5892,16 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
if (!snapshot_start_block) {
// Needed for ComputeUTXOStats to determine the
// height and to avoid a crash when base_blockhash.IsNull()
return util::Error{strprintf(Untranslated("Did not find snapshot start blockheader %s"),
base_blockhash.ToString())};
return util::Error{Untranslated(strprintf("Did not find snapshot start blockheader %s",
base_blockhash.ToString()))};
}

int base_height = snapshot_start_block->nHeight;
const auto& maybe_au_data = GetParams().AssumeutxoForHeight(base_height);

if (!maybe_au_data) {
return util::Error{strprintf(Untranslated("Assumeutxo height in snapshot metadata not recognized "
"(%d) - refusing to load snapshot"), base_height)};
return util::Error{Untranslated(strprintf("Assumeutxo height in snapshot metadata not recognized "
"(%d) - refusing to load snapshot", base_height))};
}

const AssumeutxoData& au_data = *maybe_au_data;
Expand Down Expand Up @@ -5939,12 +5939,12 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
if (coin.nHeight > base_height ||
outpoint.n >= std::numeric_limits<decltype(outpoint.n)>::max() // Avoid integer wrap-around in coinstats.cpp:ApplyHash
) {
return util::Error{strprintf(Untranslated("Bad snapshot data after deserializing %d coins"),
coins_count - coins_left)};
return util::Error{Untranslated(strprintf("Bad snapshot data after deserializing %d coins",
coins_count - coins_left))};
}
if (!MoneyRange(coin.out.nValue)) {
return util::Error{strprintf(Untranslated("Bad snapshot data after deserializing %d coins - bad tx out value"),
coins_count - coins_left)};
return util::Error{Untranslated(strprintf("Bad snapshot data after deserializing %d coins - bad tx out value",
coins_count - coins_left))};
}
coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));

Expand Down Expand Up @@ -5982,8 +5982,8 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
}
}
} catch (const std::ios_base::failure&) {
return util::Error{strprintf(Untranslated("Bad snapshot format or truncated snapshot after deserializing %d coins"),
coins_processed)};
return util::Error{Untranslated(strprintf("Bad snapshot format or truncated snapshot after deserializing %d coins",
coins_processed))};
}
}

Expand All @@ -6003,8 +6003,8 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
out_of_coins = true;
}
if (!out_of_coins) {
return util::Error{strprintf(Untranslated("Bad snapshot - coins left over after deserializing %d coins"),
coins_count)};
return util::Error{Untranslated(strprintf("Bad snapshot - coins left over after deserializing %d coins",
coins_count))};
}

LogPrintf("[snapshot] loaded %d (%.2f MB) coins from snapshot %s\n",
Expand Down Expand Up @@ -6035,8 +6035,8 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(

// Assert that the deserialized chainstate contents match the expected assumeutxo value.
if (AssumeutxoHash{maybe_stats->hashSerialized} != au_data.hash_serialized) {
return util::Error{strprintf(Untranslated("Bad snapshot content hash: expected %s, got %s"),
au_data.hash_serialized.ToString(), maybe_stats->hashSerialized.ToString())};
return util::Error{Untranslated(strprintf("Bad snapshot content hash: expected %s, got %s",
au_data.hash_serialized.ToString(), maybe_stats->hashSerialized.ToString()))};
}

snapshot_chainstate.m_chain.SetTip(*snapshot_start_block);
Expand Down
22 changes: 11 additions & 11 deletions src/wallet/feebumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
}

if (wtx.mapValue.count("replaced_by_txid")) {
errors.push_back(strprintf(Untranslated("Cannot bump transaction %s which was already bumped by transaction %s"), wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid")));
errors.push_back(Untranslated(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid"))));
return feebumper::Result::WALLET_ERROR;
}

Expand Down Expand Up @@ -74,10 +74,10 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
CFeeRate minMempoolFeeRate = wallet.chain().mempoolMinFee();

if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
errors.push_back(strprintf(
Untranslated("New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "),
Comment on lines -77 to -78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit curious about why the caliber of the scripted diff was so powerful, but I guess this multi-line case is it. Would also catch potential other cases like it which could have been merged between the parent commit of this PR and when it ended up being merged.

errors.push_back(Untranslated(
strprintf("New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- ",
FormatMoney(newFeerate.GetFeePerK()),
FormatMoney(minMempoolFeeRate.GetFeePerK())));
FormatMoney(minMempoolFeeRate.GetFeePerK()))));
return feebumper::Result::WALLET_ERROR;
}

Expand All @@ -89,7 +89,7 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans

std::optional<CAmount> combined_bump_fee = wallet.chain().calculateCombinedBumpFee(reused_inputs, newFeerate);
if (!combined_bump_fee.has_value()) {
errors.push_back(strprintf(Untranslated("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions.")));
errors.push_back(Untranslated(strprintf("Failed to calculate bump fees, because unconfirmed UTXOs depend on enormous cluster of unconfirmed transactions.")));
}
CAmount new_total_fee = newFeerate.GetFee(maxTxSize) + combined_bump_fee.value();

Expand All @@ -99,23 +99,23 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
CAmount minTotalFee = old_fee + incrementalRelayFee.GetFee(maxTxSize);

if (new_total_fee < minTotalFee) {
errors.push_back(strprintf(Untranslated("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)"),
FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))));
errors.push_back(Untranslated(strprintf("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)",
FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize)))));
return feebumper::Result::INVALID_PARAMETER;
}

CAmount requiredFee = GetRequiredFee(wallet, maxTxSize);
if (new_total_fee < requiredFee) {
errors.push_back(strprintf(Untranslated("Insufficient total fee (cannot be less than required fee %s)"),
FormatMoney(requiredFee)));
errors.push_back(Untranslated(strprintf("Insufficient total fee (cannot be less than required fee %s)",
FormatMoney(requiredFee))));
return feebumper::Result::INVALID_PARAMETER;
}

// Check that in all cases the new fee doesn't violate maxTxFee
const CAmount max_tx_fee = wallet.m_default_max_tx_fee;
if (new_total_fee > max_tx_fee) {
errors.push_back(strprintf(Untranslated("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)"),
FormatMoney(new_total_fee), FormatMoney(max_tx_fee)));
errors.push_back(Untranslated(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)",
FormatMoney(new_total_fee), FormatMoney(max_tx_fee))));
return feebumper::Result::WALLET_ERROR;
}

Expand Down
Loading
Loading