All translatable format strings are fixed. This change surfaces errors in them at compile-time.
The implementation achieves this by allowing to delay the translation (or std::string construction) that previously happened in Untranslated() or _() by returning a new type from those functions. The new type can be converted to bilingual_str where needed.
This can be tested by adding a format string error in an original string literal and observing a new compile-time failure.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31061.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, l0rinc |
| Concept ACK | stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
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;
In commit “refactor: Pick translated string after format” (fa22616839903369757069d50aa8f55e246b2b6c)
Commit message says this change is “required for a future commit” but that makes it sound this is because of some implementation detail, when I think the actual reason is that strprintf can’t check format string at compile time if it is passed a runtime std::string value.
Unless I’m missing something would be clearer if commit message just said this passes return value of the underscore function to directly to strprintf so the format string can be checked at compile time.
17@@ -18,6 +18,48 @@
18 #include <vector>
19
20 namespace util {
21+template <unsigned num_params>
22+constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
In commit “move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN” (fa6cef7cf762a33755697dff20b75a9f235e96a9)
I think it would make more sense to call this function util::detail::CheckNumFormatSpecifiers instead of util::Detail_CheckNumFormatSpecifiers
re: #31061 (review)
I don’t think this is possible with a simple diff touching only lines that are already touched in this pull request. If you disagree, I am happy to accept any commit that compiles. I am also happy to review any pull request that achieves this and is split up from this pull request.
I see what you mean: it’s not possible to introduce a util::detail namespace because it would conflict with the util::detail in the hex_literals inline namespace.
However, it doesn’t actually make sense for there to be a detail namespace in an inline namespace, and this seemslikely to create problems for callers using the inline namespace, since they may have detail namespaces of their own. This was easy enough to fix so I just fixed it in the suggested branch.
16@@ -17,12 +17,12 @@ template <unsigned NumArgs>
17 inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
18 {
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);
21+ util::Detail_CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
In commit “move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN” (fa6cef7cf762a33755697dff20b75a9f235e96a9)
Is this comment accurate? If so it seems like if is a more obvious approach to avoiding -Wunused errors would be to write (void)fmt; or just drop the fmt variable and declare the function as void PassFmt(util::ConstevalFormatString<NumArgs>)
re: #31061 (review)
Thanks, added unrelated change to fix comment.
Thanks, comment makes sense. I knew there was probably some reason for doing it that way but couldn’t figure out what it was.
106@@ -107,7 +107,7 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
107 }
108 }
109 } catch (const std::exception& e) {
110- return ConfigError{ConfigStatus::FAILED, Untranslated(e.what())};
111+ return ConfigError{ConfigStatus::FAILED, Untranslated(std::string{e.what()})};
In commit “refactor: Mark run-time Untranslated() c_str with std::string” (fa213aa88dfe9354c314a855c72ea33b3966c5c0)
I think this change is unfortunate and doesn’t make sense. It seems like it could be a avoided with a small scripted diff to replace strprintf(Untranslated("..."), ...) with Untranslated(strprintf("...", ...)) and that replacement would make the code clearer and more consistent anyway.
It seems like it could be a avoided with a small scripted diff to replace
strprintf(Untranslated("..."), ...)withUntranslated(strprintf("...", ...))and that replacement would make the code clearer and more consistent anyway.
Are you sure? The two are not equivalent, because args can also be a bilingual_str, in which case the scripted-diff does not compile. In any case, it seems unrelated to this pull request and I don’t see how it “simplifies” stuff and it touches different lines anyway, so maybe a separate issue or pull request would be better to explore this?
I think this change is unfortunate and doesn’t make sense.
Can you explain which part doesn’t make sense and how this relates to strprintf and tinyformat at all? I can’t see any tfm use in the line you commented on.
re: #31061 (review)
Thanks, it’s a good point that Untranslated(strprintf) is not equivalent to strprintf(Untranslated) in all cases. But I think those cases are basically broken because they are substituting non-english string fragments inside english strings.
I created a branch which removes these (4) cases and adds a scripted-diff to remove the remaining strprintf(Untranslated) uses. This allows a lot of simplifications to this PR:
Branch is https://github.com/ryanofsky/bitcoin/commits/pr/tfmt, commits are:
But I think those cases are basically broken because they are substituting non-english string fragments inside english strings.
I am not sure I agree. Untranslated("%s:%i ") doesn’t look English to me. To me, it looks like Untranslated() is sometimes used to annotate format strings that are valid in any language and do not need to be translated.
And your fix doesn’t prevent non-English fragments in English strings anyway, because you are using operator+ to concatenate the English/Untranslated string with a translated (_()) string.
So I am not sure your suggestion is easy for developers to follow (having to sometimes emulate formatting with operator+ and conversions, and other times not), but maybe I am missing something.
To extend a bit: Previously (and after my pull right now), it was easy to toggle between translation and non-translation by replacing _ with Untranslated (or vice-versa):
0-strprintf(Untranslated("Cannot create database file %s"), filename);
1+strprintf( _("Cannot create database file %s"), filename);
With your change, the function call order has to be changed as well.
re: #31061 (review)
I don’t see ability to swap _ and Untranslated as a real benefit here, because it is pretty easy to switch between translated and non translated format strings either way, and if you are doing that it probably makes sense to actually look at format string arguments and see if they should be translated or untranslated for consistency. The PR also already loses the ability to swap between _ and Untranslated in other places anyway since bilingual_str and Translatable structs are incompatible.
You are right that Untranslated("%s:%i ") is not an english string and the commit message in ab4e567f4c8fc2b332e4f620567fab01ede4e1f4 wasn’t doing an a good job of describing changes there. I think most of the 54 cases changed in d5a8e2b24ff46e0bc5e0fdc733663f5c5527efb0 are untranslated English strings though, and having a compile time check to make sure non-english strings aren’t inserted accidentally inside them is a good thing.
If you really disagree with this and prefer strprintf(Untranslated(…)) over Untranslated(strprintf(…)) I think we could also go in that direction with the opposite scripted-diff. I just think trying to support both and making Untranslated function return different things when it is called with a char* instead of std::string, and having to add extra casts here to work around that in this commit is unfortunate.
It seems like implementation of this PR could be simpler, API can be simpler, and code can be more consistent by having Untranslated return one thing or the other instead of both.
re: #31061 (review)
Opened #31072 to implement suggested simplification. Marking this thread resolved since it it’s not meant to block this PR in any case.
88@@ -89,8 +89,8 @@ BOOST_AUTO_TEST_CASE(check_value_or)
89 BOOST_CHECK_EQUAL(IntFn(10, false).value_or(20), 20);
90 BOOST_CHECK_EQUAL(NoCopyFn(10, true).value_or(20), 10);
91 BOOST_CHECK_EQUAL(NoCopyFn(10, false).value_or(20), 20);
92- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), Untranslated("A"));
93- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B"));
94+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), bilingual_str{Untranslated("A")});
95+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), bilingual_str{Untranslated("B")});
In commit “refactor: Delay translation of Untranslated() or _() literals” (aaaa4fb20156b4375d92e1eca4acc90a425a1896)
Looks like these changes would also not be necessary if using earlier suggestion to replace strprintf(Untranslated("..."), ...) with Untranslated(strprintf("...", ...)) so Untranslated definition does not have to change.
24+{
25+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
26+}
27+/** Type to denote whether an orginal string literal is translatable */
28+template <bool translatable = true>
29+struct Original {
In commit “refactor: Delay translation of Untranslated() or _() literals” (aaaa4fb20156b4375d92e1eca4acc90a425a1896)
Name Original is vague. Would suggest renaming Original something like bilingual_literal or bilingual_const to be consistent with bilingual_str and bilingual_fmt since it represents a bilingual literal or constant string which is the return value of the underscore function.
I also think this can just be a normal non-template struct, and bool translatable template argument can be dropped if you take eariler suggestion to replace strprintf(Untranslated("..."), ...) with Untranslated(strprintf("...", ...)) so Untranlated can continue to return bilingual_str
Translatable<bool _>. I don’t think bilingual_* makes sense, because it is only an internally used type to hold the hand of the compiler. The type is supposed to convert into a real bilingual_str either directly or via bilingual_fmt and tfm.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31292791221
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
72@@ -47,8 +73,11 @@ inline bilingual_str operator+(bilingual_str lhs, const bilingual_str& rhs)
73 return lhs;
74 }
75
76+consteval auto _(util::Original<true> str) { return str; }
In commit “refactor: Delay translation of Untranslated() or _() literals” (aaaa4fb20156b4375d92e1eca4acc90a425a1896)
Should be no need for this if take earlier suggestion to replace strprintf(Untranslated("..."), ...) with Untranslated(strprintf("...", ...))
22+ */
23+inline std::string translate(const char* lit)
24+{
25+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
26+}
27+/** Type to denote whether an orginal string literal is translatable */
typo nit
0/** Type to denote whether an original string literal is translatable */
40@@ -22,6 +41,13 @@ struct bilingual_str {
41 std::string original;
42 std::string translated;
43
44+ bilingual_str() = default;
45+ bilingual_str(std::string original, std::string translated) : original{original}, translated{translated} {}
46+ template <bool translatable>
47+ bilingual_str(util::Original<translatable> original) : original{original.lit}, translated{original.translate()}
48+ {
49+ }
In commit “refactor: Delay translation of Untranslated() or _() literals” (aaaa4fb20156b4375d92e1eca4acc90a425a1896)
I think it confuses and complicates the design for bilingual_str to be aware of Original since bilingual_str is just a simple, low-level pair of strings that shouldn’t know about formatting.
I think we do need return value of underscore function to be implicitly convertible to bilingual_str but that would be more cleanly implemented by adding an operator bilingual_str to the Original class.
Also (not sure about this, but) it might be nice to add an explicit conversion method to Original class, so we could write something like _("Error opening block database").str() instead of bilingual_str{_("Error opening block database")}
Also (not sure about this, but) it might be nice to add an explicit conversion method to Original class, so we could write something like
_("Error opening block database").str()instead ofbilingual_str{_("Error opening block database")}
Sure, I am happy to add bilingual_str str() and operator bilingual_str() to the Original type and remove the constructors from bilingual_str. However, this means that calling bilingual_str{Original{}} is now forbidden and one would have to use () over {}, or call the .str() method.
Let me know if this is fine and I should go ahead and push this diff:
0diff --git a/src/init.cpp b/src/init.cpp
1index fb2028281a..c849f83b05 100644
2--- a/src/init.cpp
3+++ b/src/init.cpp
4@@ -1261,7 +1261,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
5 return f();
6 } catch (const std::exception& e) {
7 LogError("%s\n", e.what());
8- return std::make_tuple(node::ChainstateLoadStatus::FAILURE, bilingual_str{_("Error opening block database")});
9+ return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database").str());
10 }
11 };
12 auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
13diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
14index 45337c6801..c829d56faa 100644
15--- a/src/test/result_tests.cpp
16+++ b/src/test/result_tests.cpp
17@@ -63,7 +63,7 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
18 {
19 ExpectResult(result, true, str);
20 BOOST_CHECK_EQUAL(result.has_value(), true);
21- BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
22+ BOOST_CHECK_EQUAL(result.value(), T(std::forward<Args>(args)...));
23 BOOST_CHECK_EQUAL(&result.value(), &*result);
24 }
25
26@@ -89,8 +89,8 @@ BOOST_AUTO_TEST_CASE(check_value_or)
27 BOOST_CHECK_EQUAL(IntFn(10, false).value_or(20), 20);
28 BOOST_CHECK_EQUAL(NoCopyFn(10, true).value_or(20), 10);
29 BOOST_CHECK_EQUAL(NoCopyFn(10, false).value_or(20), 20);
30- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), bilingual_str{Untranslated("A")});
31- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), bilingual_str{Untranslated("B")});
32+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), Untranslated("A").str());
33+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B").str());
34 }
35
36 BOOST_AUTO_TEST_SUITE_END()
37diff --git a/src/util/translation.h b/src/util/translation.h
38index 08df892733..a06f7b982f 100644
39--- a/src/util/translation.h
40+++ b/src/util/translation.h
41@@ -14,24 +14,6 @@
42 /** Translate a message to the native language of the user. */
43 const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
44
45-namespace util {
46-/**
47- * Translation function.
48- * If no translation function is set, simply return the input.
49- */
50-inline std::string translate(const char* lit)
51-{
52- return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
53-}
54-/** Type to denote whether an orginal string literal is translatable */
55-template <bool translatable = true>
56-struct Original {
57- const char* const lit;
58- consteval Original(const char* str) : lit{str} { assert(lit); }
59- std::string translate() const { return translatable ? util::translate(lit) : lit; }
60-};
61-} // namespace util
62-
63 /**
64 * Bilingual messages:
65 * - in GUI: user's native language + untranslated (i.e. English)
66@@ -41,13 +23,6 @@ struct bilingual_str {
67 std::string original;
68 std::string translated;
69
70- bilingual_str() = default;
71- bilingual_str(std::string original, std::string translated) : original{original}, translated{translated} {}
72- template <bool translatable>
73- bilingual_str(util::Original<translatable> original) : original{original.lit}, translated{original.translate()}
74- {
75- }
76-
77 bilingual_str& operator+=(const bilingual_str& rhs)
78 {
79 original += rhs.original;
80@@ -73,6 +48,26 @@ inline bilingual_str operator+(bilingual_str lhs, const bilingual_str& rhs)
81 return lhs;
82 }
83
84+namespace util {
85+/**
86+ * Translation function.
87+ * If no translation function is set, simply return the input.
88+ */
89+inline std::string translate(const char* lit)
90+{
91+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
92+}
93+/** Type to denote whether an orginal string literal is translatable */
94+template <bool translatable = true>
95+struct Original {
96+ const char* const lit;
97+ consteval Original(const char* str) : lit{str} { assert(lit); }
98+ std::string translate() const { return translatable ? util::translate(lit) : lit; }
99+ bilingual_str str() const { return {lit, translate()}; }
100+ operator bilingual_str() const { return str(); }
101+};
102+} // namespace util
103+
104 consteval auto _(util::Original<true> str) { return str; }
105
106 /** Mark a bilingual_str as untranslated */
re: #31061 (review)
Yes I do think that’s better and basically made the same change in my branch, though without the str() method since it seems it makes it a little less clear what return type is.
Yes I do think that’s better and basically made the same change in my branch, though without the str() method since it seems it makes it a little less clear what return type is.
Ok, pushed my diff above, with the added str() removed again.
23+{
24+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
25+}
26+/** Type to denote whether an orginal string literal is translatable */
27+template <bool translatable = true>
28+struct Original {
ConstevalStringLiteral instead of creating a new util::Original? This would 1) minimize the diff and 2) I find ConstevalStringLiteral a much more helpful name than Original.
re: #31061 (review)
in aaaa4fb: seems more straightforward to just extend
ConstevalStringLiteralinstead of creating a newutil::Original? This would 1) minimize the diff and 2) I findConstevalStringLiterala much more helpful name thanOriginal.
I think this is only a question of naming since Original class is replacing ConstevalStringLiteral this commit. In by branch I called this ConstEvalTranslated, since it’s the type name for values returned from the underscore function, and is not just referring to a generic string literal. But I didn’t put much thought into the name and something else may be better.
in aaaa4fb: seems more straightforward to just extend
ConstevalStringLiteralinstead of creating a newutil::Original? This would 1) minimize the diff and 2) I findConstevalStringLiterala much more helpful name thanOriginal.
Are you sure it would minimize the diff? Original needs to deal with the concept of translations, which seems out-of-scope for ConstevalStringLiteral. Having two separate types to only deal with the things they care about seems easier. However, I may be missing something and if you find a smaller diff, I am happy to consider it.
I’ve renamed the type to Translatable<bool _>.
strprintf(Untranslated("..."), ...) with Untranslated(strprintf("...", ...)), which would be a good change on its own for clarity and consistency. More details in comments below.
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"), QString::fromStdString(Untranslated("Wallet restored successfully \n").translate()));
0 QMessageBox::information(m_parent_widget, tr("Restore wallet message"), QString::fromStdString("Wallet restored successfully \n"));
re: #31061 (review)
Would be reasonable to change this but no more change is actually required here with my suggested branch.
re: #31061 (review)
Simplified this further in 6fb39655c5cd2bebad902d271d1edae64b817d2b from #31072. It might just be a bug that this string is not translated, and there also seems to be trailing whitespace in the string. Current commit just simplifies code and keeps existing behavior though.
37+ const bool translatable;
38+ template <bool translatable>
39+ consteval bilingual_fmt(Original<translatable> o) : original{o.lit}, translatable{translatable}
40+ {
41+ }
42+ std::string translate() { return translate(original.fmt); }
This function is currently unused, and it seems incorrectly implemented as it doesn’t compile when actually used:
0In file included from ../../src/clientversion.cpp:9:
1../../src/util/translation.h:42:48: error: too many arguments to function call, expected 0, have 1
2 std::string translate() { return translate(original.fmt); }
3 ~~~~~~~~~ ^~~~~~~~~~~~
4../../src/util/translation.h:106:42: note: in instantiation of member function 'util::bilingual_fmt<1>::translate' requested here
5 tfm::format(fmt.translate(), translate_arg(args, true)...)};
6 ^
7../../src/clientversion.cpp:74:33: note: in instantiation of function template specialization 'tinyformat::format<char[13]>' requested here
8 const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;
Suggested fix to address both:
0diff --git a/src/util/translation.h b/src/util/translation.h
1index 0f230017d9..2de99f386d 100644
2--- a/src/util/translation.h
3+++ b/src/util/translation.h
4@@ -39,7 +39,7 @@ struct bilingual_fmt {
5 consteval bilingual_fmt(Original<translatable> o) : original{o.lit}, translatable{translatable}
6 {
7 }
8- std::string translate() { return translate(original.fmt); }
9+ std::string translate() { return util::translate(original.fmt); }
10 };
11 } // namespace util
12
13@@ -103,7 +103,7 @@ bilingual_str format(util::bilingual_fmt<sizeof...(Args)> fmt, const Args&... ar
14 }
15 }};
16 return bilingual_str{tfm::format(fmt.original, translate_arg(args, false)...),
17- tfm::format(util::translate(fmt.original.fmt), translate_arg(args, true)...)};
18+ tfm::format(fmt.translate(), translate_arg(args, true)...)};
19 }
20 } // namespace tinyformat
21
re: #31061 (review)
This is a good catch, but it seems better to just drop the template method instead of trying to fix it up and use it. It just seems to add complexity and it doesn’t seem like there is another use-case for it.
Suggested fix to address both:
Good catch. Though, I don’t think the fix fixes all issues. There was a third that the translatable member field was unused. I’ve pushed a fix to fix all three issues.
23+inline std::string translate(const char* lit)
24+{
25+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
26+}
27+/** Type to denote whether an orginal string literal is translatable */
28+template <bool translatable = true>
{Translateable,Untranslateable} for readability over the non-obvious <{true,false}> boolean
nit: I would prefer templating this on an enum
{Translateable,Untranslateable}for readability over the non-obvious<{true,false}>boolean
Note: with changes in suggested branch it should no longer be necessary for this to be templated.
I’ve renamed the type to Translatable<bool _>, which should clear this up without adding an enum?
The type is only supposed to be used internally, so I can also wrap it into a detail:: if you think this is helpful?
Concept ACK, and this is a smaller diff than I’d expected to implement this, nice!
I need to think about it more and work out some code, but I feel like the new interface is not as intuitive yet as it could be. Will follow up with that in next review.
65+ std::string translate() const { return translatable ? util::translate(lit) : lit; }
66+ operator bilingual_str() const { return {lit, translate()}; }
67+};
68+} // namespace util
69+
70+consteval auto _(util::Translatable<true> str) { return str; }
In commit “refactor: Delay translation of Untranslated() or _() literals” (fa62737ae398bd56ef9e68b8d0a4d217cfcc532b)
Thinking about this more it seems like _ function does not be consteval, and could just return a template type that inherits from bilingual_str, and includes the number of format specifiers included in the string as part of its type information. This would be nice because runtime semantics of _ would be unchanged, it would still do the translation at time it is called, and there would be no need to make any translated -> translate() replacements.
I think the main downside to this approach is would be need to refactor CheckNumFormatSpecifiers but there would be fewer changes overall. The other downside would be that _ would need to be aware the translated strings might be a format string and count the specifiers instead of just passing it along blindly. Neither downside would have to exist if c++ just allowed arguments to consteval functions to be treated as compile time constants, which they are by definition, but they did not do this (I think because stylistically they want to see compile-time arguments passed as <> arguments not () arguments.)
Not sure about any of this, though, would need to experiment.
re: #31061 (review)
Not sure about any of this, though, would need to experiment.
To follow up, I was able to implement an alternate approach in #31074 that lets _ and Untranslated always return bilingual_str instances that you can access with .original and .translated instead of .lit and translate(). The resulting API seems more clean and consistent, but it does require messier syntax when calling strprintf (implmented in a scripted diff).
I’m not really sure about tradeoffs of messier syntax vs cleaner API, and I’m guessing there’s some other way to make the API more consistent.
Code review ACK fae6acacc0216aa07f5a4a0ee75fbade620d7023. This is a nice improvement overall, and definitely better than the status quo due to extra type checking it provides. I do think there is extra complexity here and extra changes that could go away if we adopt #31072 and stop passing untranslated format strings to strprintf.
I also think it probably would be nicer if _() would continue to look up translations right away, instead of delaying them until conversion to bilingual str. I think it could return a bilingual_str with extra compile-time information instead of returning different type with .lit and .translate() fields. I’m not sure this is possible though, and it’s likely there’s something I’m overlooking.
EDIT: I am implemented an approach in #31074 that makes _() and Untranslated() both return bilingual_str with .original and .translated fields and no not delay the time of translation. Unfortunately, because C++ does not currently allow marking function parameters constexpr, it means strings needs to be passed as template parameters instead of function parameters, resulting in a messier syntax, even though the implementation and API are simpler. I closed the PR because it is not a clear win over this one.
Code review ACK 2c15a858ca7b3f14eaebf4b39134cd0e9c559273. No changes since the last review other than rebase.
I do think it would simplify this PR and make review more meaningful if #31072 could be merged first. This PR is combining application code changes which require looking at surrounding code and seeing if the changes fit in, with translation class changes which require thinking about API design. I think it’s harder to evaluate both things clearly when they are bundled in the same PR. I wouldn’t object to this PR going in first though since it has been open longer.
This passes the return value of _() directly to strprintf so the format
string can be checked at compile time in a future commit.
This is required for a future commit. Can be reviewed via the git
options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Also move util::detail::Hex to a proper namespace instead of an inline
namespace so it doesn't conflict with the new util::detail namespace, and
won't create other problems for callers trying to use the inline namespaces.
Also fix a misleading comment in util_string_tests.cpp.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
Requested by clang-tidy:
src/wallet/salvage.cpp:119:18: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
119 | warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
| ^~~~~~~~~~
| emplace_back(
This is a no-op right now, because a std::string was constructed either
way. However, it is needed for a future commit that forces
Untranslated() char literals to be evaluated at compile time.
This is required for a future commit that requires Untranslated() and
_() to be consteval for format literals.