refactor: Avoid std::string format strings

maflcko commented at 11:47 am on November 14, 2024: member

This changes some unchecked std::string format strings to use string literals, which are consteval checked at compile-time.

Split out, because it is used in several pull requests.

Pass literal format strings instead of std::string so formats can be
checked at compile time.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>

fa1177e3d7

DrahtBot commented at 11:47 am on November 14, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31287.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, tdb3, rkrux, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Refactoring on Nov 14, 2024

in src/wallet/wallet.cpp:1900 in fa1177e3d7

1896@@ -1897,7 +1897,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1897                     fast_rescan_filter ? "fast variant using block filters" : "slow variant inspecting all blocks");
1898 
1899     fAbortRescan = false;
1900-    ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)
1901+    ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)

laanwj commented at 12:40 pm on November 14, 2024:

It seems unnecessary to use strprintf at all just to concatenate two strings and a space 😄 Edit: oh, it’s a number, never mind. Edit.2: wait, no, it isn’t, the 0 is another argument to ShowProgress. AHH.

ryanofsky commented at 1:56 pm on November 15, 2024:

re: #31287 (review)

I think this confusion only backs up the original comment that:

0ShowProgress(GetDisplayName() + " " + _("Rescanning…").translated, 0);

is clearer than:

0ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), 0);

maflcko commented at 2:32 pm on November 15, 2024:

I’ll leave as-is for now. I think both are fine.

l0rinc commented at 2:54 pm on November 14, 2024: contributor

ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

tdb3 approved

tdb3 commented at 3:01 am on November 15, 2024: contributor

code review and light test ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

Quick checks:

Viewed the -printpriority description with bitcoind -help -help-debug
Induced test failure in txrequest_tests to see the string.

rkrux approved

rkrux commented at 6:07 am on November 15, 2024: none

tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

Make and unit tests successful. Checked the printpriority arg message in help.

0  -printpriority
1       Log transaction fee rate in BTC/kvB when mining blocks (default: 0)

maflcko added this to the milestone 29.0 on Nov 15, 2024

maflcko requested review from ryanofsky on Nov 15, 2024

in src/wallet/wallet.cpp:1915 in fa1177e3d7

1911@@ -1912,7 +1912,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1912             m_scanning_progress = 0;
1913         }
1914         if (block_height % 100 == 0 && progress_end - progress_begin > 0.0) {
1915-            ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
1916+            ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));

ryanofsky commented at 1:52 pm on November 15, 2024:

In commit “refactor: Avoid std::string format strings” (fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d)

Note: Precedes this PR, but there appears to be mixed translation bugs in all the wallet code changed here. GetDisplayName() can return the english string “[default wallet]” while “Rescanning” is translated. This might become a non-issue after the legacy wallet removal where (I’m assuming) unnamed wallets will finally get names? I think having more type safety to make it harder to accidentally combine translated and untranslated strings would be good thing in general, though.

maflcko commented at 2:31 pm on November 15, 2024:

Maybe a follow-up can fix this, so that this remains a refactor?

ryanofsky commented at 5:14 pm on November 15, 2024:

Maybe a follow-up can fix this, so that this remains a refactor?

Thanks, I experimented with various ways to clean up this show progress code, and I think it would probably be best if the string formatting was done in the GUI. This would require interface class changes we probably want anyway, but would be a bigger change. For now just went with a simple bugfix translating “default wallet” to match the rest of the string in #31296

ryanofsky approved

ryanofsky commented at 2:07 pm on November 15, 2024: contributor

Code review ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

I think I’ll merge this PR shortly so we can make progress on related PRs #31061 and #31072, but you can let me know if you would prefer this not to be merged now in case you want to address the comment that using strprintf("%s %s", ...) instead of + makes code more confusing

ryanofsky merged this on Nov 15, 2024

ryanofsky closed this on Nov 15, 2024

maflcko deleted the branch on Nov 15, 2024

ryanofsky referenced this in commit 87152db666 on Nov 15, 2024

refactor: Avoid std::string format strings #31287

Code Coverage & Benchmarks

Reviews