refactor: Avoid std::string format strings #31287

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2411-check-fmt changing 4 files +9 −9
  1. 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.

  2. refactor: Avoid std::string format strings
    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
  3. DrahtBot commented at 11:47 AM on November 14, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

  4. DrahtBot added the label Refactoring on Nov 14, 2024
  5. 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:

    ShowProgress(GetDisplayName() + " " + _("Rescanningā€¦").translated, 0);

    is clearer than:

    ShowProgress(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.

  6. l0rinc commented at 2:54 PM on November 14, 2024: contributor

    ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

  7. tdb3 approved
  8. 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.
  9. rkrux approved
  10. rkrux commented at 6:07 AM on November 15, 2024: contributor

    tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

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

      -printpriority
       Log transaction fee rate in BTC/kvB when mining blocks (default: 0)
  11. maflcko added this to the milestone 29.0 on Nov 15, 2024
  12. maflcko requested review from ryanofsky on Nov 15, 2024
  13. 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

  14. ryanofsky approved
  15. 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

  16. ryanofsky merged this on Nov 15, 2024
  17. ryanofsky closed this on Nov 15, 2024
  18. maflcko deleted the branch on Nov 15, 2024
  19. ryanofsky referenced this in commit 87152db666 on Nov 15, 2024
  20. ryanofsky referenced this in commit df45123dc0 on Nov 18, 2024
  21. TheCharlatan referenced this in commit bd64a555d7 on Dec 3, 2024
  22. TheCharlatan referenced this in commit 15319cfcd5 on Dec 4, 2024
  23. stickies-v referenced this in commit f6b6da7935 on Dec 18, 2024
  24. stickies-v referenced this in commit 6816a65596 on Dec 18, 2024
  25. stickies-v referenced this in commit de777e8ed6 on Dec 18, 2024
  26. ryanofsky referenced this in commit 34c611a99d on Jan 15, 2025
  27. ryanofsky referenced this in commit 370abf849b on Jan 15, 2025
  28. ryanofsky referenced this in commit 49c79a2216 on Jun 2, 2025
  29. ryanofsky referenced this in commit 01737883b3 on Jun 2, 2025
  30. achow101 referenced this in commit 73972d5617 on Aug 12, 2025
  31. janus referenced this in commit fc91d5364a on Sep 14, 2025
  32. bug-castercv502 referenced this in commit c87bcd9110 on Sep 28, 2025
  33. bitcoin locked this on Nov 15, 2025
Contributors
maflckoDrahtBotlaanwjryanofskyl0rinctdb3rkrux
Labels
Refactoring

Milestone
29.0

Linked (view graph)
#31296 wallet: Translate [default wallet] string in progress messages
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me