refactor: Clean up messy strformat and bilingual

ryanofsky commented at 12:19 pm on October 11, 2024: contributor

This PR cleans up string formatting in the codebase so other PRs adding compile time checking can be simpler and easier to review (specifically #30928, #31061, #31074, and #31149).

Currently these PRs are hard to review because in addition to changing formatting APIs, they have to update callers that are using the API’s in unusual ways. Clean up these callers now so later PRs can be simpler. Specifically:

Use string literals instead of std::string format strings to enable more compile-time checking.
Avoid using untranslated bilingual strings as format strings. Use originals so they can by checked at compile time.
Favor Untranslated(strprintf(...)) over strprintf(Untranslated(...), ...) for consistency and to prevent translated and untranslated strings from being unintentionally combined.

DrahtBot commented at 12:19 pm on October 11, 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/31072.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, l0rinc
Concept ACK	hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#31061 (refactor: Check translatable format strings at compile-time by maflcko)
#30988 (Split CConnman by vasild)
#29770 (index: Check all necessary block data is available before starting to sync by fjahr)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Oct 11, 2024

in src/init.cpp:905 in 19f49f138c outdated

900@@ -901,7 +901,9 @@ bool AppInitParameterInteraction(const ArgsManager& args)
901     // Warn if unrecognized section name are present in the config file.
902     bilingual_str warnings;
903     for (const auto& section : args.GetUnrecognizedSections()) {
904-        warnings += strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized.") + Untranslated("\n"), section.m_file, section.m_line, section.m_name);
905+        warnings += Untranslated(strprintf("%s:%i ", section.m_file, section.m_line)) +
906+                    strprintf(_("Section [%s] is not recognized."), section.m_name) +

maflcko commented at 2:00 pm on October 11, 2024:

in commit " refactor: Use + instead of strformat to concatenate Untranslated strings “:

I still don’t think this is correct. You are using + to concatenate Untranslated and _() (translated), possibly mixing English strings with non-English ones. Also, this compiles, so there are no compile-time checks preventing the “mix”.

ryanofsky commented at 2:24 pm on October 11, 2024:

I still don’t think this is correct.

What part of this is not correct? Is there a specific sentence or something?

You are using + to concatenate Untranslated and _() (translated), possibly mixing English strings with non-English ones. Also, this compiles, so there are no compile-time checks preventing the “mix”.

There is nothing in this commit message (19f49f138c4756b5219e715d8749d15be07e2c81) which mentions mixing English or non-english strings.

As a whole, this PR is updating around 30 strprintf calls to be more readable and consistent and to, yes, make 27 of them now trigger compiler errors if there is an attempt to pass a bilingual_str as a strprintf parameter, preventing accidental mixing English and non-english strings.

Obviously it should not be compile error to intentionally mix translated and non-translated strings, and this is still possible with +, which the code is now using in 3 more places.

maflcko commented at 2:31 pm on October 11, 2024:

I still don’t think this is correct.

What part of this is not correct? Is there a specific sentence or something?

Sorry for being unclear. The commit title says

“refactor: Use + instead of strformat to concatenate Untranslated strings”.

It should say:

“refactor: Use + instead of strformat to concatenate bilingual strings, which may be Untranslated or translated (_)”

maflcko commented at 2:06 pm on October 11, 2024: member

I don’t mind the changes here, but I don’t think the motivation is correct and it seems verbose and misleading.

I don’t think it matters much whether this is merged or not, but you could just replace the motivation with:

“This replacement makes the implementation of some compile-time checks in the future easier. Also, it makes the code a bit more consistent in that Untranslated isn’t allowed as a format-string anymore, only plain std::strings, or translated strings. The downside would be that developers may have to use operator+ more often, making it a bit harder to write code involving formatting of (un)translated strings. However, if it becomes too annoying, the changes here should be easy to revert.”

ryanofsky renamed this:
~~scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))~~
refactor: Clean up messy strformat and bilingual_str usages
on Oct 12, 2024

ryanofsky force-pushed on Oct 12, 2024

ryanofsky commented at 1:40 am on October 12, 2024: contributor

Updated 9a101d2928d14e39365064dc976615b4b9674ca5 -> 022ffd149a64f944d1ade75664e8dc19d04bcda7 (pr/bfmt.1 -> pr/bfmt.2, compare) to include more cleanups and help simplify implementation of compile-time translated string format checking in #31061 or #31074

re: #31072#pullrequestreview-2362888700

I don’t mind the changes here, but I don’t think the motivation is correct and it seems verbose and misleading.

I don’t know what was misleading, but I rewrote description and will paste old text in comment below in case it’s useful. Sorry for verbosity, I like to think by writing, but it doesn’t always result in the most helpful thing to read.

scripted-diff: Replace strprintf(Untranslated(…)) with Untranslated(strprintf(…))

This PR is just a scripted-diff replacing strprintf calls like:

0strprintf(Untranslated("Error parsing command line arguments: %s"), error))

with:

0Untranslated(strprintf("Error parsing command line arguments: %s", error))

Currently code is inconsistent and uses a mix of both styles, but second style is better because:

It scans more cleanly. It is easier to read the strprintf call, and easier to think about because the whole string, not just one part, is passed to Untranslated() to convert from std::string to bilingual_str.
It lets the compiler prevent janky UX where non-english fragments are inserted into english messages. One potential case where a bilingual_str was substituted into an english format string is fixed in the first commit of this PR.
It doesn’t waste work formatting the same string twice.
It simplifies the translation API after #31061. Instead of Untranslated() function sometimes returning a bilingual_str, other times returning a Translatable<false> struct with a different set of fields, it can stay the way it is and simply convert std::string to bilingual_str.
It simplifies the implementation of #31061, allowing two commits faf71be89ad9d07c0848d13da51fc71c11395984 and fae6acacc0216aa07f5a4a0ee75fbade620d7023 to be dropped, as well as other changes in other commits.

One potential downside of this change, pointed out #31061 (review), is that after this PR if you want to change an untranslated format string to a translated one (or vice versa), you will have to to add _() around the format string and drop Untranslate() around the formatted string, instead of just literally replacing Untranslated with _. But I think this is good, because the change is still easy to make, and it lets you consider whether other parts of the strprintf call also should switch to translated strings to avoid jank of non-translated messages being embedded in translated strings (or vice versa).

An alternative to this PR could be a scripted-diff switching all code to use strprintf(Untranslated(...)) instead of Untranslated(strprintf(...)). I think this alternative would also be an improvement over the status quo, since it would also make the code more consistent, also simplify #31061, and also avoid needing to overload Untranslated() there to return objects with incompatible types. But I think this PR is better than the alternative for reasons 1-3 above, and better because it lets Untranslated remain a simple one-liner that converts a std::string to bilingual_str and is not involved in formatting.

Note: The scripted diff in this PR only removes current strprintf(Untranslated(...)) calls without adding enforcement to prevent new ones. But adding enforcement is easy with #31061 and simplifies that PR by dropping support for the strprintf(Untranslated(...)) syntax.

in src/wallet/rpc/backup.cpp:537 in faa34ad0a7 outdated

533@@ -534,7 +534,7 @@ RPCHelpMan importwallet()
534 
535         // Use uiInterface.ShowProgress instead of pwallet.ShowProgress because pwallet.ShowProgress has a cancel button tied to AbortRescan which
536         // we don't want for this progress bar showing the import progress. uiInterface.ShowProgress does not have a cancel button.
537-        pwallet->chain().showProgress(strprintf("%s " + _("Importing…").translated, pwallet->GetDisplayName()), 0, false); // show progress dialog in GUI
538+        pwallet->chain().showProgress(strprintf("%s %s", pwallet->GetDisplayName(), _("Importing…").translated), 0, false); // show progress dialog in GUI

maflcko commented at 12:38 pm on October 15, 2024:

nit in faa34ad0a7f5271958e5313e7aae2baaed41587e: Since you include the commit here, but just as a note: The commit message isn’t 100% correct. Should there be an error in the format string, the ConstevalFormatString fails substitution and leads to the std::string one being selected.

This commit (message) only becomes true after commit 4ce60d82a0c1e21ee7ecba69e4c8926720829429 from #30928. So I wonder if 4ce60d82a0c1e21ee7ecba69e4c8926720829429+faa34ad0a7f5271958e5313e7aae2baaed41587e could be split up into a first pull request, as everything else is based on it?

stickies-v commented at 2:52 pm on October 24, 2024:

This commit (message) only becomes true after commit 4ce60d8 from #30928. So I wonder if 4ce60d8+faa34ad0a7f5271958e5313e7aae2baaed41587e could be split up into a first pull request, as everything else is based on it?

This is now ~done in https://github.com/bitcoin/bitcoin/pull/31149

ryanofsky commented at 6:48 pm on October 29, 2024:

re: #31072 (review)

Thanks, updated the commit message to avoid being misleading, and can update it again if one of the other PRs gets merged first.

in src/qt/walletcontroller.cpp:434 in 6fb39655c5 outdated

430@@ -431,7 +431,7 @@ void RestoreWalletActivity::finish()
431         QMessageBox::warning(m_parent_widget, tr("Restore wallet warning"), QString::fromStdString(Join(m_warning_message, Untranslated("\n")).translated));
432     } else {
433         //: Title of message box which is displayed when the wallet is successfully restored.
434-        QMessageBox::information(m_parent_widget, tr("Restore wallet message"), QString::fromStdString(Untranslated("Wallet restored successfully \n").translated));
435+        QMessageBox::information(m_parent_widget, tr("Restore wallet message"), "Wallet restored successfully \n");

maflcko commented at 12:43 pm on October 15, 2024:

nit in 6fb39655c5cd2bebad902d271d1edae64b817d2b: Are you sure this change is correct? I’d presume this was intentionally written to denote an Untranslated string that would otherwise be translated. Alternatively, it seems like an oversight that should be translated?

My preference would be to leave this as-is for now.

ryanofsky commented at 6:54 pm on October 29, 2024:

re: #31072 (review)

Makes sense. Just dropped this commit even though it is a refactoring, because I don’t want to make a different change that would change behavior.

in src/validation.cpp:5814 in fd188650f0 outdated

5759@@ -5760,7 +5760,7 @@ util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
5760 
5761     if (auto res{this->PopulateAndValidateSnapshot(*snapshot_chainstate, coins_file, metadata)}; !res) {
5762         LOCK(::cs_main);
5763-        return cleanup_bad_snapshot(strprintf(Untranslated("Population failed: %s"), util::ErrorString(res)));
5764+        return cleanup_bad_snapshot(Untranslated(strprintf("Population failed: %s", util::ErrorString(res).original)));

maflcko commented at 12:46 pm on October 15, 2024:

fd188650f0aefced09fdb5c3d21c960791104774: Just for reference, everything in this call graph is untranslated, so if there was a translation, it would already be a bug, and this patch wouldn’t fix that bug.

ryanofsky commented at 6:56 pm on October 29, 2024:

re: #31072 (review)

Thanks added a note to clarify. The point of this change is to prevent a future change in a different part of the call graph from causing a bug here, but there is no bug currently.

in src/validation.cpp:6145 in fdd1992e44 outdated

6090@@ -6091,7 +6091,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
6091 
6092         auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk();
6093         if (!rename_result) {
6094-            user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result));
6095+            user_error += Untranslated("\n") + util::ErrorString(rename_result);

maflcko commented at 12:57 pm on October 15, 2024:

fdd1992e44c4eb49b802aad30c12638cf92fcf8e: I am thinking that strprintf(Untranslated("something"), ...) is something that may be used normally. Right now, it may not be used extensively, or only used to concatenate two blobs. However, I can imagine that in the future someone may want to write strprintf(Untranslated("<a href="...">%s</a><br>%s<br>"), ...) to format some bilingual strings into html, which would be a bit more verbose by doing with plenty of +.

Not saying that this is something common, but I wanted to mention it.

ryanofsky commented at 7:21 pm on October 29, 2024:

re: #31072 (review)

Thanks, we also talked about this offline and I agree that strprintf can often be nicer than concatenation for building strings, but I think this approach doesn’t work very well for untranslated bilingual strings. For example in the HTML case, you would probably only want to format the translated strings as HTML, not both translated and original strings. I think the downsides of supporting untranslated format strings outweigh the upsides. (Downsides being having two inconsistent ways of formatting the same strings, having a more complicated interface, and potential for introducing mixed translation bugs.)

maflcko approved

maflcko commented at 12:58 pm on October 15, 2024: member

lgtm. Left some comments for context.

DrahtBot added the label Needs rebase on Oct 28, 2024

hebasto commented at 2:37 pm on October 29, 2024: member

Concept ACK.

ryanofsky force-pushed on Oct 29, 2024

ryanofsky commented at 7:59 pm on October 29, 2024: contributor

Rebased 022ffd149a64f944d1ade75664e8dc19d04bcda7 -> 46c55ab427c0771cacaed7a20612ffe2cc3543db (pr/bfmt.2 -> pr/bfmt.3, compare) due to conflict with #31042, also addressing review comments.

DrahtBot removed the label Needs rebase on Oct 29, 2024

fanquake referenced this in commit f44e39c9d0 on Nov 14, 2024

ryanofsky force-pushed on Nov 15, 2024

ryanofsky commented at 3:04 am on November 15, 2024: contributor

Updated 46c55ab427c0771cacaed7a20612ffe2cc3543db -> 1e9ea7de0b5ee6d3179930d4fc93c8e276e525bd (pr/bfmt.3 -> pr/bfmt.4, compare) with no changes. Just updated commit messages to be more accurate now that #31174 is merged.

ryanofsky force-pushed on Nov 15, 2024

ryanofsky commented at 4:39 pm on November 15, 2024: contributor

Rebased 1e9ea7de0b5ee6d3179930d4fc93c8e276e525bd -> 85df2fbf26c73f97f85797868155247c11a4ccd6 (pr/bfmt.4 -> pr/bfmt.5, compare) after #31287 was merged (just for clarity, there were no merge conflicts)

in src/node/interface_ui.cpp:77 in 85df2fbf26 outdated

73@@ -74,7 +74,7 @@ bool InitError(const bilingual_str& str, const std::vector<std::string>& details
74     // functions which provide error details are ones that run during early init
75     // before the GUI uiInterface is registered, so there's no point passing
76     // main messages and details separately to uiInterface yet.
77-    return InitError(details.empty() ? str : strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details)));
78+    return InitError(details.empty() ? str : str + Untranslated(strprintf(":\n%s", MakeUnorderedList(details))));

l0rinc commented at 9:56 am on November 17, 2024:

nit: it seems to me both start with str and optionally extend with the details, if available. Maybe we could tell this story with the code as well:

0auto suffix = details.empty() ? bilingual_str{} : Untranslated(strprintf(":\n%s", MakeUnorderedList(details)));
1return InitError(str + suffix);

ryanofsky commented at 1:32 pm on November 18, 2024:

re: #31072 (review)

I think the advantage of your suggestion is avoids repeating str. So I’d probably write the code that way if str was something more complicated than a single variable.

I think the story of the current version is “this returns message plus a list of details if any” and the story of the suggested version is “this returns a message plus a suffix, which can be empty or can be a list of details” and I think both are reasonable, but the current version does seem a little simpler to me and more similar to current code.

in src/validation.cpp:6145 in 85df2fbf26 outdated

6119@@ -6120,7 +6120,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation()
6120 
6121         auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk();
6122         if (!rename_result) {
6123-            user_error = strprintf(Untranslated("%s\n%s"), user_error, util::ErrorString(rename_result));
6124+            user_error += Untranslated("\n") + util::ErrorString(rename_result);

l0rinc commented at 10:20 am on November 17, 2024:

I don’t fully understand why this couldn’t be an Untranslated(strprintf (is it because bilingual_str user_error doesn’t have an std::ostream operator defined?), but if you touch again, consider simplifying to + ErrorString(rename_result) (or extending Result with an error returning method as well, but that’s likely outside the scope of this PR), i.e.

0if (auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); !rename_result) {
1    user_error += Untranslated("\n") + rename_result.error();
2}

with

 0template <class M>
 1class Result
 2{
 3    // ...
 4    const bilingual_str& error() const
 5    {
 6        assert(!*this);
 7        return std::get<0>(m_variant);
 8    }
 9};
10
11template <typename T>
12bilingual_str ErrorString(const Result<T>& result)
13{
14    return result ? bilingual_str{} : result.error();
15}

ryanofsky commented at 1:29 pm on November 18, 2024:

re: #31072 (review)

I don’t fully understand why this couldn’t be an Untranslated(strprintf

Good catch! Thanks for the close reading. The commit description 75643fd325748f8b81dc78edfc1266f47990f9b4 was wrong when it said that this change was needed to let the scripted diff compile. That comment is only true for previous commit d392068d7257e0cb8ca8e95fcb5e504b4b4f2839, not this one. Fixed the description now.

About the Result class, it intentionally does not have a bilingual_str accessor. The goal of the Result class is to provide meaningful error messages to users when operations fail. While useful error messages can be a represented by a single bilingual_str they can also be represented by a lists of errors and warnings, or lists of errors with details for debugging, or other richer ways that can be shown in GUIs or returned as json. In #25665 the Result class is changed to allow custom error representations and change the default representation to support multiple errors and warnings. Making the ErrorString a nonmember function instead of a member function makes this more easily possible because the Result class only needs to be concerned with representing errors and not formatting them.

in src/clientversion.cpp:74 in 85df2fbf26 outdated

70@@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const
71 
72 std::string CopyrightHolders(const std::string& strPrefix)
73 {
74-    const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION);
75+    const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;

l0rinc commented at 10:41 am on November 17, 2024:

I think I’m missing some important detail here, but I see that #define COPYRIGHT_HOLDERS_FINAL "The Bitcoin Core developers" is basically #define COPYRIGHT_HOLDERS "The %s developers" substituted into #define COPYRIGHT_HOLDERS_SUBSTITUTION "Bitcoin Core", could we maybe simplify to

0const auto copyright_devs = _(COPYRIGHT_HOLDERS_FINAL).translated;

or would that prevent translation?

maflcko commented at 11:15 am on November 17, 2024:

I don’t think you’d want to translate the name of the program? For reference, I’ve also split up the common refactor commits into #31295, as they are needed by two pull requests.

ryanofsky commented at 2:23 pm on November 18, 2024:

re: #31072 (review)

Right, I think it would be bad if changing or tweaking the name of the program required newly translating “The %s developers” with the new name of the program hardcoded. Code should say _(COPYRIGHT_HOLDERS) instead of _(COPYRIGHT_HOLDERS_FINAL) so translators only have to translate the original string, not the expanded string.

in src/init.cpp:889 in 85df2fbf26 outdated

883@@ -884,7 +884,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
884     }
885     bilingual_str errors;
886     for (const auto& arg : args.GetUnsuitableSectionOnlyArgs()) {
887-        errors += strprintf(_("Config setting for %s only applied on %s network when in [%s] section.") + Untranslated("\n"), arg, ChainTypeToString(chain), ChainTypeToString(chain));
888+        errors += strprintf(_("Config setting for %s only applied on %s network when in [%s] section."), arg, ChainTypeToString(chain), ChainTypeToString(chain)) + Untranslated("\n");

l0rinc commented at 10:45 am on November 17, 2024:

👍, a lot cleaner this way

in src/init.cpp:904 in 85df2fbf26 outdated

898@@ -899,7 +899,7 @@ bool AppInitParameterInteraction(const ArgsManager& args)
899     // Warn if unrecognized section name are present in the config file.
900     bilingual_str warnings;
901     for (const auto& section : args.GetUnrecognizedSections()) {
902-        warnings += strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized.") + Untranslated("\n"), section.m_file, section.m_line, section.m_name);
903+        warnings += Untranslated(strprintf("%s:%i ", section.m_file, section.m_line)) + strprintf(_("Section [%s] is not recognized."), section.m_name) + Untranslated("\n");

l0rinc commented at 10:50 am on November 17, 2024:

👍, it’s obvious that the Section name is m_name, not m_file.

nit: it seems to me in other cases we’re using %d instead of %i (if you edit again, consider simplifying to %d which seems to have fewer surprises): https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L986

in test/lint/run-lint-format-strings.py:16 in b8bf7bf55b outdated

12@@ -13,7 +13,7 @@
13 import sys
14 
15 FALSE_POSITIVES = [
16-    ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"),
17+    ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"),

l0rinc commented at 11:01 am on November 17, 2024:

what is the reason for these still being false positives?

ryanofsky commented at 1:31 pm on November 18, 2024:

re: #31072 (review)

what is the reason for these still being false positives?

IIUC, linter is just very simple and doesn’t expand macros. If I remove this line running the linter shows:

0src/clientversion.cpp: Expected 0 argument(s) after format string but found 1 argument(s): strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)

in src/wallet/feebumper.cpp:27 in d0695fe71c outdated

23@@ -24,24 +24,24 @@ namespace wallet {
24 static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
25 {
26     if (wallet.HasWalletSpend(wtx.tx)) {
27-        errors.push_back(Untranslated("Transaction has descendants in the wallet"));
28+        errors.emplace_back(Untranslated("Transaction has descendants in the wallet"));

l0rinc commented at 11:02 am on November 17, 2024:

nit: it’s simple enough to review, but if you edit again consider a scripted diff

l0rinc approved

l0rinc commented at 11:08 am on November 17, 2024: contributor

ACK 85df2fbf26c73f97f85797868155247c11a4ccd6

I have basically redone the change locally to understand it better and mostly left questions or nits, I’m fine with merging as is

DrahtBot requested review from hebasto on Nov 17, 2024

ryanofsky force-pushed on Nov 18, 2024

ryanofsky commented at 2:26 pm on November 18, 2024: contributor

Thanks for the reviews!

Updated 85df2fbf26c73f97f85797868155247c11a4ccd6 -> 3b24c24889c09f6c8f33408b57c2042c4e565388 (pr/bfmt.5 -> pr/bfmt.6, compare) fixing a commit description (no code changes)

l0rinc commented at 3:35 pm on November 18, 2024: contributor

ACK 3b24c24889c09f6c8f33408b57c2042c4e565388

refactor: Avoid concatenation of format strings

Instead just concatenate already formatted strings. This allows untranslated
format strings to be checked at compile time now, and translated format strings
to be checked at compile time in #31061.

058021969b

refactor: Don't embed translated string in untranslated string.

This could produce an english error message containing non-english string
fragments if PopulateAndValidateSnapshot started returning any translated
strings in the future. This change is also needed to make the next
scripted-diff commit work.

831d2bfcf9

refactor: Use + instead of strformat to concatenate translated & untranslated strings

This change manually removes two strprintf(Untranslated...) calls. All
remaining calls are removed in the next scripted-diff commit.

Removing these calls makes code more consistent and makes it easier to
implement compile-time checking enforcing that format strings contain valid
specifiers, by avoiding the need for the Untranslated() function to be involved
in formatting.

Additionally, using + and += instead of strprintf here makes code a little
shorter, and more type-safe because + unlike strprintf only works on strings of
the same type, making it less likely english strings and bilingual strings will
be unintentionally combined.

006e4d1d59

scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf)

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-

0184d33b3d

ryanofsky force-pushed on Dec 4, 2024

ryanofsky commented at 9:08 pm on December 4, 2024: contributor

Rebased 3b24c24889c09f6c8f33408b57c2042c4e565388 -> 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 (pr/bfmt.6 -> pr/bfmt.7, compare) with no changes to simplify after #31295

maflcko commented at 8:20 pm on December 5, 2024: member

lgtm ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 🔹

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 🔹
3z0E0h3Qb/aDEKJqchivwiOliKiEH2e+LZo6omZpyBzBgdPrlsBhmnxX+Q6JrVbQg5jglHOg6YU3NUgI7A4T/Aw==

DrahtBot requested review from l0rinc on Dec 5, 2024

l0rinc commented at 11:09 am on December 6, 2024: contributor

ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 - no overall difference because of the rebase

fanquake merged this on Dec 6, 2024

fanquake closed this on Dec 6, 2024

in src/wallet/feebumper.cpp:78 in 0184d33b3d

73@@ -74,10 +74,10 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans
74     CFeeRate minMempoolFeeRate = wallet.chain().mempoolMinFee();
75 
76     if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
77-        errors.push_back(strprintf(
78-            Untranslated("New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- "),

hodlinator commented at 10:46 pm on December 6, 2024:

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.

hodlinator commented at 10:48 pm on December 6, 2024: contributor

post-merge-ACK

Really cleaned the slate nicely for #31061.

refactor: Clean up messy strformat and bilingual_str usages #31072

Code Coverage & Benchmarks

Reviews

Conflicts