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 _() by returning a new type from this function. 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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31061.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, ryanofsky |
| Stale ACK | l0rinc, hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
Thanks, fixed commit message.
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
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.
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.
Thanks, replaced my commit with your 71d382650b5a0bfb21085e8f6bd22152d2148459
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>)
Thanks, added unrelated change to fix comment.
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):
-strprintf(Untranslated("Cannot create database file %s"), filename);
+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.
Thanks. Now that it is merged, I went ahead and pushed commit 08bab0447027ace65f18a72c98b1c87683f8f378 from #31061 (review) into this pull (rebased)
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.
(same)
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
Ok, renamed to 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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/31292791221</sub>
<details><summary>Hints</summary>
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.
</details>
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
/** Type to denote whether an original string literal is translatable */
thanks, fixed
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:
diff --git a/src/init.cpp b/src/init.cpp
index fb2028281a..c849f83b05 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1261,7 +1261,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
return f();
} catch (const std::exception& e) {
LogError("%s\n", e.what());
- return std::make_tuple(node::ChainstateLoadStatus::FAILURE, bilingual_str{_("Error opening block database")});
+ return std::make_tuple(node::ChainstateLoadStatus::FAILURE, _("Error opening block database").str());
}
};
auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
index 45337c6801..c829d56faa 100644
--- a/src/test/result_tests.cpp
+++ b/src/test/result_tests.cpp
@@ -63,7 +63,7 @@ void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args
{
ExpectResult(result, true, str);
BOOST_CHECK_EQUAL(result.has_value(), true);
- BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
+ BOOST_CHECK_EQUAL(result.value(), T(std::forward<Args>(args)...));
BOOST_CHECK_EQUAL(&result.value(), &*result);
}
@@ -89,8 +89,8 @@ BOOST_AUTO_TEST_CASE(check_value_or)
BOOST_CHECK_EQUAL(IntFn(10, false).value_or(20), 20);
BOOST_CHECK_EQUAL(NoCopyFn(10, true).value_or(20), 10);
BOOST_CHECK_EQUAL(NoCopyFn(10, false).value_or(20), 20);
- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), bilingual_str{Untranslated("A")});
- BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), bilingual_str{Untranslated("B")});
+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), Untranslated("A").str());
+ BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B").str());
}
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/translation.h b/src/util/translation.h
index 08df892733..a06f7b982f 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -14,24 +14,6 @@
/** Translate a message to the native language of the user. */
const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
-namespace util {
-/**
- * Translation function.
- * If no translation function is set, simply return the input.
- */
-inline std::string translate(const char* lit)
-{
- return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
-}
-/** Type to denote whether an orginal string literal is translatable */
-template <bool translatable = true>
-struct Original {
- const char* const lit;
- consteval Original(const char* str) : lit{str} { assert(lit); }
- std::string translate() const { return translatable ? util::translate(lit) : lit; }
-};
-} // namespace util
-
/**
* Bilingual messages:
* - in GUI: user's native language + untranslated (i.e. English)
@@ -41,13 +23,6 @@ struct bilingual_str {
std::string original;
std::string translated;
- bilingual_str() = default;
- bilingual_str(std::string original, std::string translated) : original{original}, translated{translated} {}
- template <bool translatable>
- bilingual_str(util::Original<translatable> original) : original{original.lit}, translated{original.translate()}
- {
- }
-
bilingual_str& operator+=(const bilingual_str& rhs)
{
original += rhs.original;
@@ -73,6 +48,26 @@ inline bilingual_str operator+(bilingual_str lhs, const bilingual_str& rhs)
return lhs;
}
+namespace util {
+/**
+ * Translation function.
+ * If no translation function is set, simply return the input.
+ */
+inline std::string translate(const char* lit)
+{
+ return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
+}
+/** Type to denote whether an orginal string literal is translatable */
+template <bool translatable = true>
+struct Original {
+ const char* const lit;
+ consteval Original(const char* str) : lit{str} { assert(lit); }
+ std::string translate() const { return translatable ? util::translate(lit) : lit; }
+ bilingual_str str() const { return {lit, translate()}; }
+ operator bilingual_str() const { return str(); }
+};
+} // namespace util
+
consteval auto _(util::Original<true> str) { return str; }
/** 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 {
in aaaa4fb20156b4375d92e1eca4acc90a425a1896: seems more straightforward to just extend 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 _>.
Code review ACK fabb28f6c8f772d38917a23dfbda706292070ba1. This looks great, and implementation is very clean. However, I think it could be simplified significantly if there were a scripted diff commit to replace 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()));
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.
I don't like this suggestion. If you disagree, you can continue discussion in #31072 (review)
The hunk isn't needed anymore, so I dropped it.
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:
In file included from ../../src/clientversion.cpp:9:
../../src/util/translation.h:42:48: error: too many arguments to function call, expected 0, have 1
std::string translate() { return translate(original.fmt); }
~~~~~~~~~ ^~~~~~~~~~~~
../../src/util/translation.h:106:42: note: in instantiation of member function 'util::bilingual_fmt<1>::translate' requested here
tfm::format(fmt.translate(), translate_arg(args, true)...)};
^
../../src/clientversion.cpp:74:33: note: in instantiation of function template specialization 'tinyformat::format<char[13]>' requested here
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;
Suggested fix to address both:
<details> <summary>git diff on fad211bfef</summary>
diff --git a/src/util/translation.h b/src/util/translation.h
index 0f230017d9..2de99f386d 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -39,7 +39,7 @@ struct bilingual_fmt {
consteval bilingual_fmt(Original<translatable> o) : original{o.lit}, translatable{translatable}
{
}
- std::string translate() { return translate(original.fmt); }
+ std::string translate() { return util::translate(original.fmt); }
};
} // namespace util
@@ -103,7 +103,7 @@ bilingual_str format(util::bilingual_fmt<sizeof...(Args)> fmt, const Args&... ar
}
}};
return bilingual_str{tfm::format(fmt.original, translate_arg(args, false)...),
- tfm::format(util::translate(fmt.original.fmt), translate_arg(args, true)...)};
+ tfm::format(fmt.translate(), translate_arg(args, true)...)};
}
} // namespace tinyformat
</details>
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>
nit: I would prefer templating this on an enum {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.
concept ack
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.
Concept ACK, nice to see these verifications being done earlier
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.
Code review ACK f4df783f1ca22d96476d52ec5d1929547691ba13. Just rebased and reordered commit since last review
utACK f4df783f1ca22d96476d52ec5d1929547691ba13
72 | + const bool translatable; 73 | + template <bool translatable> 74 | + consteval bilingual_fmt(Translatable<translatable> o) : original{o.lit}, translatable{translatable} 75 | + { 76 | + } 77 | + std::string translate() { return translatable ? util::translate(original.fmt) : original.fmt; }
nit:
std::string translate() const { return translatable ? util::translate(original.fmt) : original.fmt; }
removed the function instead
11 | @@ -12,10 +12,10 @@ BOOST_AUTO_TEST_SUITE(translation_tests) 12 | BOOST_AUTO_TEST_CASE(translation_namedparams) 13 | { 14 | bilingual_str arg{"original", "translated"}; 15 | - bilingual_str format{"original [%s]", "translated [%s]"}; 16 | + constexpr auto format{Untranslated("original [%s]")};
nit: Can't we just be explicit here in the test to document what is happening, since it is slightly tricky?
constexpr util::Translatable<false> format{Untranslated("original [%s]")};
I think the type is an implementation detail and doesn't matter, as long as it compiles. One could even inline this line into the next line, but I wanted to keep the diff minimal.
ACK f4df783f1ca22d96476d52ec5d1929547691ba13
Increased validation of format strings. :+1:
Maybe amend PR summary or final commit by spelling out something like:
Delays bilingual_str-construction until the end of the overload of tinyformat::format in translation.h. The fmt parameter to the overload is changed from bilingual_str -> util::bilingual_fmt, the latter being a new type which contains a ConstevalFormatString. bilingual_fmt in turn is only constructible from the new util::Translatable type, which is only returned by calling one of:
Translatable<true> _(const char*)Translatable<false> Untranslated(const char*)Iff you re-touch, consider making code a bit less template-heavy by splitting Translatable in two:
diff --git a/src/util/translation.h b/src/util/translation.h
index 2da4f68e55..657cd21f9c 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -58,31 +58,32 @@ inline std::string translate(const char* lit)
{
return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(lit) : lit;
}
-/** Type to denote whether an original string literal is translatable */
-template <bool translatable = true>
struct Translatable {
const char* const lit;
consteval Translatable(const char* str) : lit{str} { assert(lit); }
- std::string translate() const { return translatable ? util::translate(lit) : lit; }
+ std::string translate() const { return util::translate(lit); }
operator bilingual_str() const { return {lit, translate()}; }
};
+struct Untranslatable {
+ const char* const lit;
+ consteval Untranslatable(const char* str) : lit{str} { assert(lit); }
+ operator bilingual_str() const { return {lit, lit}; }
+};
template <unsigned num_params>
struct bilingual_fmt {
const ConstevalFormatString<num_params> original;
const bool translatable;
- template <bool translatable>
- consteval bilingual_fmt(Translatable<translatable> o) : original{o.lit}, translatable{translatable}
- {
- }
+ consteval bilingual_fmt(Translatable o) : original{o.lit}, translatable{true} {}
+ consteval bilingual_fmt(Untranslatable o) : original{o.lit}, translatable{false} {}
std::string translate() { return translatable ? util::translate(original.fmt) : original.fmt; }
};
} // namespace util
-consteval auto _(util::Translatable<true> str) { return str; }
+consteval auto _(util::Translatable str) { return str; }
/** Mark a bilingual_str as untranslated */
inline bilingual_str Untranslated(std::string original) { return {original, original}; }
-consteval util::Translatable<false> Untranslated(const char* original) { return original; }
+consteval util::Untranslatable Untranslated(const char* original) { return original; }
// Provide an overload of tinyformat::format which can take bilingual_str arguments.
namespace tinyformat {
Should this PR still be in Draft-status @maflcko?
addressed feedback, rebased
69 | +template <unsigned num_params> 70 | +struct bilingual_fmt { 71 | + const ConstevalFormatString<num_params> original; 72 | + consteval bilingual_fmt(Translatable o) : original{o.lit} 73 | + { 74 | + }
nit: Since previous review, this constructors function prototype has shrunk enough to keep it on one line, as you are doing for Translatable just above.
consteval bilingual_fmt(Translatable o) : original{o.lit} {}
thx, clang-formatted
65 | + std::string translate() const { return util::translate(lit); } 66 | + operator bilingual_str() const { return {lit, translate()}; } 67 | +}; 68 | + 69 | +template <unsigned num_params> 70 | +struct bilingual_fmt {
nit: Use Consistent naming convention in newer code as recommended by developer-notes?
struct BilingualFmt {
thx, done
Code reviewed fa6b1d2a26e25e3e6a4ef37835426371001d01ae
Really nice how you could slim this down after #31072 merged, removing Translatable<false> from this PR as after that PR Untranslated() is no longer sent into strprintf/tfm::format. Now we only need the two Translatable and bilingual_fmt as separate types as a consequence of all _()-calls not being used as format strings.
Only blocker for me is that the commit message in aaaa7e3f285d5d0ff55df4dfe050334215e2c582 still states:
This is required for a future commit that requires Untranslated() and _() to be consteval for format literals.
Untranslated() is not made consteval in this PR.
178 | @@ -179,13 +179,19 @@ namespace tfm = tinyformat; 179 | 180 | namespace tinyformat { 181 | 182 | +// Similar to std::runtime_format from C++26.
nit:
+// Added for Bitcoin Core.
Maybe use some comment style to group the modifications to avoid repeating this, similar to https://www.doxygen.nl/manual/grouping.html
thx, done
190 | // formatting without compile time checks.
191 | template <unsigned num_params>
192 | struct FormatStringCheck {
193 | consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString<num_params>{str}.fmt} {}
194 | - FormatStringCheck(const std::string& str) : fmt{str.c_str()} {}
195 | + FormatStringCheck(const RuntimeFormat& run) : fmt{run.fmt.c_str()} {}
Just realized there's some risk of dangling pointers.
FormatStringCheck(const RuntimeFormat& run LIFETIMEBOUND) : fmt{run.fmt.c_str()} {}
Also, if you're going to rename this one str->run, maybe call the other one comp or something?
LIFETIMEBOUND
Added =delete as an alternative instead.
re: #31061 (review)
Delete seems good, but I think maybe LIFETIMEBOUND could still be helpful because fmt member here is being assigned the result of run.fmt.c_str(), so the RuntimeFormat object lifetime needs to be at least as long as FormatStringCheck object lifetime, and maybe LIFETIMEBOUND could help enforce this? I don't know all of the details about how LIFETIMEBOUND works though, so this is just a question.
Ah, I forgot to remove the & in this line here, for it to have any effect. (force pushed to fix it)
My understanding is that the only way to construct a RuntimeFormat, whose copy and move ctors are deleted, would be in the FormatStringCheck(RuntimeFormat run) constructor itself. Thus, the string will be part of the same full-expression, and there are no lifetimebound issues coming from the runtime format string.
Obviously there will still be lifetime issues from the FormatStringCheck, but they exist even in C++26. See https://godbolt.org/z/Y4oxr3jxb . Also, they are not detected by the liftetimebound attribute, currently. My understanding is that temporal safety in C++ is still being worked on.
If you want, I can delete the copy/move ctors of FormatStringCheck as well, but this seems like a pre-existing issue, which will also be re-introduced when switching to C++26 formatting.
Reverted to the previously reviewed version. I don't think LIFETIMEBOUND is useful here, because:
RuntimeFormat strings should be rare either way, and even rarer for developers to be touched.<!-- https://godbolt.org/z/Tfa5rsTab
re: #31061 (review)
I'm still not sure I see any downsides to adding LIFETIMEBOUND. It seems like a great way to document the fact that these functions require long-lived references, and to provide instant feedback for code and design bugs without needing to create a PR and wait for an ASAN job to fail.
I also don't understand why the additional temporaries and moves and elisions in fad431e548a694600a3fe212127052e5f922a622 would preferable to using references and creating the minimum number of objects with straightforward full-expression lifetimes.
fad431e548a694600a3fe212127052e5f922a622 also seems unsafe because it doesn't prevent lvalues from being passed to the FormatStringCheck constructor, and if an lvalue (or any other argument incompatible with copy elision) is passed it will result in FormatStringCheck::fmt being assigned a dangling pointer.
Would suggest following changes as a simpler alternative:
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -141,6 +141,7 @@ namespace tfm = tinyformat;
//------------------------------------------------------------------------------
// Implementation details.
+#include <attributes.h> // Added for Bitcoin Core
#include <algorithm>
#include <iostream>
#include <sstream>
@@ -181,10 +182,8 @@ namespace tinyformat {
// Added for Bitcoin Core. Similar to std::runtime_format from C++26.
struct RuntimeFormat {
- std::string fmt; // Not a string view, because tinyformat requires a c_str
- explicit RuntimeFormat(std::string str) : fmt{std::move(str)} {}
- RuntimeFormat(const RuntimeFormat&) = delete;
- RuntimeFormat& operator=(const RuntimeFormat&) = delete;
+ const std::string& fmt; // Not a string view, because tinyformat requires a c_str
+ explicit RuntimeFormat(LIFETIMEBOUND const std::string& str) : fmt{str} {}
};
// Added for Bitcoin Core. Wrapper for checking format strings at compile time.
@@ -193,7 +192,7 @@ struct RuntimeFormat {
template <unsigned num_params>
struct FormatStringCheck {
consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString<num_params>{str}.fmt} {}
- FormatStringCheck(const RuntimeFormat& run) : fmt{run.fmt.c_str()} {}
+ FormatStringCheck(LIFETIMEBOUND const RuntimeFormat& run) : fmt{run.fmt.c_str()} {}
FormatStringCheck(util::ConstevalFormatString<num_params> str) : fmt{str.fmt} {}
operator const char*() { return fmt; }
const char* fmt;
I'm still not sure I see any downsides to adding LIFETIMEBOUND.
Apart from the downsides I mentioned, LIFETIMEBOUND also isn't supported on gcc. So using vanilla C++17 (or 20) seems more portable to issue compiler errors at compile time, but I don't think it matters much in this case anyway.
LIFETIMEBOUND only being supported by one compiler isn't a great reason to skip adding it. Good to let Clang do what it can, and also provide documentation to devs. Not blocking for me though.
Tried the following, but Clang 18 compiles without any warnings when adding LIFETIMEBOUND.
std::optional<tinyformat::FormatStringCheck<0>> fsc;
{
auto fmt{std::make_unique<tinyformat::RuntimeFormat>("{}")};
fsc = {tinyformat::FormatStringCheck<0>(*fmt)};
}
Also tried removing the RuntimeFormat-ctor from FormatStringCheck and found that it's only being used in tinyformat::format(util::BilingualFmt... outside off fuzz-tests. So this should be a minor concern.
LIFETIMEBOUNDonly being supported by one compiler isn't a great reason to skip adding it.
It is just one reason, among many that have been mentioned previously.
Tried the following, but Clang 18 compiles without any warnings when adding LIFETIMEBOUND.
Yes, I mentioned that the attribute right now doesn't really do any useful checks at compile time in this context. (It is more useful in other contexts). This is one of the reasons why I preferred to use an approach that works at compile time on all compilers, only using the standard language.
Though, I don't think it matters much in this case anyway, so anything is fine here.
re: #31061 (review)
Apart from the downsides I mentioned,
It is just one reason, among many that have been mentioned previously.
Thanks for adding LIFETIMEBOUND here. I've been pretty confused by this discussion because I haven't seen any reasons given for why it would actually be better not to use LIFETIMEBOUND, and I don't know of any reasons. I understand LIFETIMEBOUND doesn't work everywhere, but don't understand why that matters as long as it catches bugs and provides fast feedback in the places it does work, and is helpful everywhere to document references that need to be long-lived.
If LIFETIMEBOUND triggered false positive errors like clang thread safety annotations do, that could be a real downside and reason not to use it. But as far as I know it doesn't cause harms or have any negative effects like that, so I've been confused by the pushback here. Maybe the reasons not to use it are as simple as "looks ugly" / "adds noise" / "seems useless" which are perfectly ok (and are also reasons I don't use const various places). i'd be fine with keeping or dropping LIFETIMEBOUND in this PR, just wondering if I am not understanding real problems with it.
I just think that compile failures are better than runtime errors or UB. Using vanilla C++17 code without attributes to turn bad code (such as https://godbolt.org/z/6336nGKcM (replace 0 with 1 to enable the compile-time checks)) into a compile failure on all compilers seems better than a runtime error, possibly only in CI.
Oh thanks for explaining that. I thought you were only disabling copies and assignment to prevent expensive copies due to not passing RuntimeFormat by reference, and that you were not passing RuntimeFormat by reference to avoid needing to think about lifetime issues.
But actually your goal is to make RuntimeFormat run("fmt"); strprintf(run); a compiler error, and deleting the copy and assignment operators are a way to do that. So LIFETIMEBOUND isn't harmful, but just not as good
as a potentially safer solution fad431e548a694600a3fe212127052e5f922a622 that doesn't use LIFETIMEBOUND.
I think I can see now how fad431e548a694600a3fe212127052e5f922a622 actually is a safer solution in every case, and relying on mandatory copy elision is ok there because deleting the RuntimeFormat copy constructor should make it impossible to pass as a function argument without copy elision. This is more complicated than just using references, but does seem safer.
https://godbolt.org/z/6336nGKcM (replace
0with1to enable the compile-time checks)) into a compile failure on all compilers seems better than a runtime error, possibly only in CI.
With the #if set to 1 and lines 42-43 changed to
check = RuntimeFormat("fmt");
One still gets an ASAN error instead of a compile time error.
I think it will always be possible to trick a C++ compiler into UB without a warning, possibly even all currently released compilers.
If you wanted to solve the same problem for FormatStringCheck, you could apply the same approach (explicit+delete+copy-elision). However the patch would be too verbose for this codebase to be worth it.
Also, the approach can only be applied in this context. And if you want to solve lifetime issues wholesale, it will need to happen in the language itself. Personally I liked the safe % references, which would allow new C++ code to be safe. However, I don't think the proposal https://safecpp.org/draft-lifetimes.html will be in C++29, and possibly not make it at all.
178 | @@ -179,13 +179,19 @@ namespace tfm = tinyformat; 179 | 180 | namespace tinyformat { 181 | 182 | +// Similar to std::runtime_format from C++26. 183 | +struct RuntimeFormat { 184 | + explicit RuntimeFormat(std::string str) : fmt{std::move(str)} {} 185 | + std::string fmt;
Noticed that the std-version uses string_view. Maybe add a note about why you didn't go with that in the commit message, or consider switching.
thx, added comment
Thanks for incorporating my previous feedback!
Leaving some brief comments on the new changes before getting back to the weekend.
Was lint-format-strings.py picked up by some glob-expression in CI?
This looks pretty good but I think the API can be improved. Specifically I think .translate() suffixes should not be necessary and just calling _() should be sufficient to indicate that you want a translated std::string or bilingual_str. If you don't want the translation, there should be no reason to call _()
A suggested implementation dropping .translate() suffixes is: branch, diff
This also adds a little more test coverage so the original and translated format strings in the test are not the same and test can confirm the right strings are picked.
74 | +} // namespace util 75 | + 76 | +consteval auto _(util::Translatable str) { return str; } 77 | + 78 | /** Mark a bilingual_str as untranslated */ 79 | inline bilingual_str Untranslated(std::string original) { return {original, original}; }
slightly unrelated: can we use a const reference here instead?
inline bilingual_str Untranslated(const std::string& original) { return {original, original}; }
Yes, it is possible, but adding const prevents a move, if someone wanted to write return {original, std::move(original)};
Undefined parameter evaluation order would make it unclear which of them should be explicitly moved though? https://stackoverflow.com/a/2934909
I hope the compiler introduces an implicit move for whichever parameter it deems to be the last use of the variable.
Undefined parameter evaluation order would make it unclear which of them should be explicitly moved though?
Starting with C++17, return {original, std::move(original)}; is both legal and correct. See https://timsong-cpp.github.io/cppwp/n4868/dcl.init.general#18
However, I'd leave this as-is for now, as the performance shouldn't matter here, even if the compiler can't optimize it out.
Was
lint-format-strings.pypicked up by some glob-expression in CI?
Yes, see:
In the future it could make sense to explicitly list them one-by-one in the list, instead of using the implicit glob.
Code review ACK fa7cb525b7069ce0197d2f309102432273fea4d6. LGTM!
Looks like there is a codegen bug in msvc, or some other issue? It would be good if someone with a Windows box could take a look, to either debug the crash or create a minimal reproducer.
Looks like there is a codegen bug in msvc, or some other issue? It would be good if someone with a Windows box could take a look, to either debug the crash or create a minimal reproducer.
I think maybe the previous fa37083b37e6 FormatStringCheck(const RuntimeFormat& run) was safe and current fad431e548a6 FormatStringCheck(RuntimeFormat run) is buggy and the msvc error is a real bug.
Previously it was safe to call run.fmt.c_str() because run object was temporary but had a lifetime was as long as the full-expression. Now it seems run object is a copy of original RuntimeFormat object and only has a lifetime as long as the FormatStringCheck constructor.
Not sure why this wouldn't cause errors on other platforms. One thing that may be unique about the timeoffsets test case that is failing is that the format string is very long so maybe this is affected by a short string optimization.
Not sure why this wouldn't cause errors on other platforms. One thing that may be unique about the timeoffsets test case that is failing is that the format string is very long so maybe this is affected by a short string optimization.
That seems unlikely, for several reasons:
TranslatedLiteral -> BilingualFmt -> ConstevalFormatString -> FormatStringCheck (only the last step is done at runtime). However, no step involves a runtime format string, or a std::string constructor.std::unique_ptr<std::string> should sidestep the short-string optimization, putting even short strings into newly allocated memory. Thus, the problem should be visible on other platforms, or consistently on Windows.
- However, no step involves a runtime format string, or a
std::stringconstructor.
Doesn't the fact that it is translated mean that is uses a runtime format string and std::string? The original string will be a const char* but the string returned from the translation function will be a runtime std::string
Ok, fair enough. Though, I still don't see how the RuntimeFormat string is destructed before the FormatStringCheck, given that:
RuntimeFormat is marked explicit, meaning that it has to be constructed before being passed to FormatStringCheckFormatStringCheck destruction.std::unique_ptr<std::string> test should disable short-string optimizations and trigger the issue reliably, if it existedSee also the godbolt: https://godbolt.org/z/M3fEnd8Wq
Maybe I am missing something obvious, but if there is no bug in msvc, it would be good to know the exact bug in this code.
Maybe I am missing something obvious, but if there is no bug in msvc, it would be good to know the exact bug in this code.
I think the following line from fad431e548a694600a3fe212127052e5f922a622 is buggy because it is saving the result of c_str() call from a string that about to be destroyed:
FormatStringCheck(RuntimeFormat run) : fmt{run.fmt.c_str()} {}
The run.fmt string object will be destroyed when the FormatStringCheck constructor finishes because run is a non-reference parameter, so it's a effectively a local variable within FormatStringCheck method.
I think the following line from https://github.com/bitcoin/bitcoin/commit/fad431e548a694600a3fe212127052e5f922a622 is buggy because it is saving the result of c_str() call from a string that about to be destroyed:
Looking at this more I think the constructor is not buggy in c++17 as long as a temporary object is being passed as the constructor argument, because the mandatory copy elision described at the top of https://en.cppreference.com/w/cpp/language/copy_elision should apply, so the temporary should not be copied, and the temporary should have the lifetime of the full-expression. So maybe MSVC is at fault here, either for not applying mandatory copy-elision, or for not extending the lifetime of the constructor argument to outlive the constructor when it does apply copy-elision.
I do think the FormatStringCheck constructor in fad431e548a694600a3fe212127052e5f922a622 is inherently fragile though, because it will be copying the constructor argument if a non-rvalue is passed, and calling .c_str() on the temporary copy that is about to be destroyed. It seems better to just use references and LIFETIMEBOUND as originally suggested and avoid temporaries and copies.
I'd prefer it if every TranslatedLiteral instance didn't have a function pointer riding on the side, but I do like the added testability.
One could make the test-implementation of G_TRANSLATION_FUN support hooking it for specific tests. It still comes with some added overhead when it comes to benchmarks and fuzzing over current master though.
<details> <summary>Diff</summary>
diff --git a/src/test/translation_tests.cpp b/src/test/translation_tests.cpp
index 2ebde95259..bcbfe9db34 100644
--- a/src/test/translation_tests.cpp
+++ b/src/test/translation_tests.cpp
@@ -2,6 +2,7 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#include <test/util/setup_common.h>
#include <tinyformat.h>
#include <util/translation.h>
@@ -9,21 +10,24 @@
BOOST_AUTO_TEST_SUITE(translation_tests)
-static TranslateFn translate{[](const char * str) { return strprintf("t(%s)", str); }};
-
BOOST_AUTO_TEST_CASE(translation_namedparams)
{
- constexpr util::TranslatedLiteral format{"original [%s]", &translate};
+ assert(!G_TRANSLATION_FUN_HOOK);
+ G_TRANSLATION_FUN_HOOK = [](const char * str) { return strprintf("t(%s)", str); };
+
+ constexpr util::TranslatedLiteral format{"original [%s]"};
bilingual_str arg{"original", "translated"};
bilingual_str result{strprintf(format, arg)};
BOOST_CHECK_EQUAL(result.original, "original [original]");
BOOST_CHECK_EQUAL(result.translated, "t(original [translated])");
- util::TranslatedLiteral arg2{"original", &translate};
+ util::TranslatedLiteral arg2{"original"};
bilingual_str result2{strprintf(format, arg2)};
BOOST_CHECK_EQUAL(result2.original, "original [original]");
BOOST_CHECK_EQUAL(result2.translated, "t(original [t(original)])");
+
+ G_TRANSLATION_FUN_HOOK = {};
}
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 12feba09a5..947f1e1ca7 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -72,7 +72,11 @@ using node::LoadChainstate;
using node::RegenerateCommitments;
using node::VerifyLoadedChainstate;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+std::function<std::string(const char*)> G_TRANSLATION_FUN_HOOK;
+
+const std::function<std::string(const char*)> G_TRANSLATION_FUN{[] (const char* str) {
+ return G_TRANSLATION_FUN_HOOK ? G_TRANSLATION_FUN_HOOK(str) : str;
+}};
constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index b0f7bdade2..f033f20dc8 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -43,6 +43,8 @@ extern const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUM
/** Retrieve the unit test name. */
extern const std::function<std::string()> G_TEST_GET_FULL_NAME;
+extern std::function<std::string(const char*)> G_TRANSLATION_FUN_HOOK;
+
static constexpr CAmount CENT{1000000};
/** Register common test args. Shared across binaries that rely on the test framework. */
diff --git a/src/util/translation.h b/src/util/translation.h
index 3a9316479d..d618b22ee2 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -13,8 +13,7 @@
#include <string>
/** Translate a message to the native language of the user. */
-using TranslateFn = std::function<std::string(const char*)>;
-const extern TranslateFn G_TRANSLATION_FUN;
+const extern std::function<std::string(const char*)> G_TRANSLATION_FUN;
/**
* Bilingual messages:
@@ -54,10 +53,9 @@ namespace util {
//! Compile-time literal string that can be translated with an optional translation function.
struct TranslatedLiteral {
const char* const original;
- const TranslateFn* translate_fn;
- consteval TranslatedLiteral(const char* str, const TranslateFn* fn = &G_TRANSLATION_FUN) : original{str}, translate_fn{fn} { assert(original); }
- operator std::string() const { return translate_fn && *translate_fn ? (*translate_fn)(original) : original; }
+ consteval TranslatedLiteral(const char* str) : original{str} { assert(original); }
+ operator std::string() const { return G_TRANSLATION_FUN ? G_TRANSLATION_FUN(original) : original; }
operator bilingual_str() const { return {original, std::string{*this}}; }
};
</details>
8 | @@ -9,13 +9,21 @@ 9 | 10 | BOOST_AUTO_TEST_SUITE(translation_tests) 11 | 12 | +static TranslateFn translate{[](const char * str) { return strprintf("t(%s)", str); }}; 13 | + 14 | BOOST_AUTO_TEST_CASE(translation_namedparams) 15 | { 16 | + constexpr util::TranslatedLiteral format{"original [%s]", &translate};
In commit "refactor: Check translatable format strings at compile-time" (fa948257f332772f058cabffe0e313a0518da724)
Could consider making the test a little more realistic with:
--- a/src/test/translation_tests.cpp
+++ b/src/test/translation_tests.cpp
@@ -11,17 +11,22 @@ BOOST_AUTO_TEST_SUITE(translation_tests)
static TranslateFn translate{[](const char * str) { return strprintf("t(%s)", str); }};
+// Custom translation function _t(), similar to _() but internal to this test.
+static inline consteval auto _t(util::TranslatedLiteral str)
+{
+ str.translate_fn = &translate;
+ return str;
+}
+
BOOST_AUTO_TEST_CASE(translation_namedparams)
{
- constexpr util::TranslatedLiteral format{"original [%s]", &translate};
-
bilingual_str arg{"original", "translated"};
- bilingual_str result{strprintf(format, arg)};
+ bilingual_str result{strprintf(_t("original [%s]"), arg)};
BOOST_CHECK_EQUAL(result.original, "original [original]");
BOOST_CHECK_EQUAL(result.translated, "t(original [translated])");
util::TranslatedLiteral arg2{"original", &translate};
- bilingual_str result2{strprintf(format, arg2)};
+ bilingual_str result2{strprintf(_t("original [%s]"), arg2)};
BOOST_CHECK_EQUAL(result2.original, "original [original]");
BOOST_CHECK_EQUAL(result2.translated, "t(original [t(original)])");
}
Code review ACK fa7cb525b7069ce0197d2f309102432273fea4d6. Looks good but did leave some suggestions which could be saved for a follow-up.
re: #31061 (comment)
I'd prefer it if every
TranslatedLiteralinstance didn't have a function pointer riding on the side, but I do like the added testability.
I don't think the TranslatedLiteral::translate_fn members should add overhead because TranslatedLiteral constructor is consteval, and in any case TranslatedLiteral objects are ephemeral, they exist only to call a translation function one time and then disappear. (The additional pointers also technically aren't function pointers, but pointers to a std::function object.)
The goal of the pointers isn't just improve testability but also to provide extensibility, so for example a libbitcoinkernel application could use define its own _ translation function and seamlessly use strprintf with its own translated strings. Having the pointers also avoids the need to mutate shared global state during tests so, for example, unlike with the suggested change, other tests could run in parallel with the translation test without being affected, and if translation test throws, it will not leave G_TRANSLATION_FUN pointing at the wrong function.
I don't think the
TranslatedLiteral::translate_fnmembers should add overhead because TranslatedLiteral constructor is consteval
Good point, I glossed over TranslatedLiteral being consteval.
Having the pointers also avoids the need to mutate shared global state during tests
Current CTest executes tests in parallel through starting separate processes, but it might change at some point and keeping it less brittle is good.
5 | @@ -6,12 +6,15 @@ 6 | #define BITCOIN_UTIL_TRANSLATION_H 7 | 8 | #include <tinyformat.h> 9 | +#include <util/string.h>
nit: Arguably tinyformat is more external than util/string.h, but our copy lives with our code so it's certainly not clear-cut.
#include <util/string.h>
#include <tinyformat.h>
re-ACK fa7cb525b7069ce0197d2f309102432273fea4d6
git range-diff master fa6b1d2 fa7cb52
Really like how _() is sufficient without .translated/.translate() thanks to TranslatedLiterals operator std::string.
Checked out fa37083b37e613f0987fe74e0397d9ed90589e4b.
Ran linter being removed in following commit (fa7cb525b7069ce0197d2f309102432273fea4d6):
â‚¿ test/lint/lint-format-strings.py
src/test/fuzz/util/wallet.h: Expected 0 argument(s) after format string but found 2 argument(s): strprintf(tfm::RuntimeFormat{desc_fmt}, "[5aa9973a/66h/4h/2h]" + seed_insecure, int{internal})
src/test/translation_tests.cpp: Expected 0 argument(s) after format string but found 1 argument(s): strprintf(format, arg2)
Might want to do something about that if we want all commits to pass, not blocking for me though.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/34246270269</sub>
<details><summary>Hints</summary>
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.
</details>
nit: Noticed a typo in the linter removal commit message:
"The linter has many >implmentation< bugs and missing features."
96 | + return arg.original; 97 | + } else { 98 | + return arg; 99 | + } 100 | + }}; 101 | + const auto translate_arg{[](const auto& arg) -> const auto& {
In commit "refactor: Check translatable format strings at compile-time" (fa0b20a1fa7b4e360b54ab06991d441e6d685aeb)
Not important, but probably would have made sense to call this translated_arg instead of translate_arg since this is not actually translating the argument or calling the translation function, but just returning a reference to translated or translatable version of the argument.
thx, done
Code review ACK fadd2e69c6176ba684c1fd06fbd765be6fb6d647. Just rebased and added lifetimebound and test suggestions since last review. Appreciate your patience with all the review comments and iterations of this PR. Will be really nice to see this merged with the new compiler checks being safer and more convenient than current runtime errors.
re-ACK fadd2e69c6176ba684c1fd06fbd765be6fb6d647
git range-diff master fa7cb52 fadd2e6
LIFETIMEBOUND to at least serve as developer documentation even though Clang isn't able to fully use it.nit: Did not fix typo: #31061 (comment)
re-ACK fa6d9a29162fb508385201c926ec745d071086fc
git range-diff master fadd2e6 fa6d9a2
Code review ACK fa6d9a29162fb508385201c926ec745d071086fc. Just rebase, spelling fix and suggested rename since last review.
185 | + const std::string& fmt; // Not a string view, because tinyformat requires a c_str 186 | + explicit RuntimeFormat(LIFETIMEBOUND const std::string& str) : fmt{str} {} 187 | +}; 188 | + 189 | // Added for Bitcoin Core. Wrapper for checking format strings at compile time. 190 | // Unlike ConstevalFormatString this supports std::string for runtime string
nit: this supports std::string for runtime string - perhaps worth updating to reflect that it now requires to be wrapped in RuntimeFormat?
I was hoping that "runtime string formatting" is clear enough to show the connection to RuntimeFormat and then looking at the RuntimeFormat constructor, one can see std::string. So I didn't change it, but I am happy to take suggestions.
Yeah it's fine as is too. The change I had in mind was just adding RunTimeWrapped-wrapped in case users just read the docstring and don't understand why their std::string argument fails.
// Added for Bitcoin Core. Wrapper for checking format strings at // compile time. Unlike ConstevalFormatString this supports // RunTimeFormat-wrapped std::string for runtime string formatting // without compile time checks.
thx, pushed doc fixup
Approach ACK fa6d9a29162fb508385201c926ec745d071086fc. Apologies for the slow re-review, but I like the latest form of this PR much more than the previous one I looked at, nice! I'll complete my full re-review in the next couple of days.
This is required for a future commit that requires _() to be consteval
for format literals.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
12 | #include <functional> 13 | #include <string> 14 | 15 | /** Translate a message to the native language of the user. */ 16 | -const extern std::function<std::string(const char*)> G_TRANSLATION_FUN; 17 | +using TranslateFn = std::function<std::string(const char*)>;
in fad276ad6518a394d065943d458e8c7cce3f997c
nit: could-reuse TranslateFn:
<details> <summary>git diff on fad276ad65</summary>
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 5c5965245b..1b2d97faff 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -50,7 +50,7 @@ using util::ToString;
// just use a plain system_clock.
using CliClock = std::chrono::system_clock;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index a627c24e86..e2b3a3b554 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -41,7 +41,7 @@ static bool fCreateBlank;
static std::map<std::string,UniValue> registers;
static const int CONTINUE_EXECUTION=-1;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
static void SetupBitcoinTxArgs(ArgsManager &argsman)
{
diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp
index ff2e4fb800..10967bd573 100644
--- a/src/bitcoin-util.cpp
+++ b/src/bitcoin-util.cpp
@@ -26,7 +26,7 @@
static const int CONTINUE_EXECUTION=-1;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
static void SetupBitcoinUtilArgs(ArgsManager &argsman)
{
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
index 033cf7a0f3..c37f9d2995 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -26,7 +26,7 @@
using util::Join;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
static void SetupWalletToolArgs(ArgsManager& argsman)
{
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index 5d5f06127d..d210e2c8ba 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -34,7 +34,7 @@
using node::NodeContext;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
#if HAVE_DECL_FORK
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index bb101ba186..9096b6f71d 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1,10 +1,11 @@
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#include <util/translation.h>
#include <functional>
#include <string>
// Define G_TRANSLATION_FUN symbol in libbitcoinkernel library so users of the
// library aren't required to export this symbol
-extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+extern const TranslateFn G_TRANSLATION_FUN{nullptr};
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
index 16befd99e8..649fead901 100644
--- a/src/qt/main.cpp
+++ b/src/qt/main.cpp
@@ -13,7 +13,7 @@
#include <string>
/** Translate string to current locale using Qt. */
-extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
+extern const TranslateFn G_TRANSLATION_FUN = [](const char* psz) {
return QCoreApplication::translate("bitcoin-core", psz).toStdString();
};
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 12feba09a5..9c9981b4a1 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -72,7 +72,7 @@ using node::LoadChainstate;
using node::RegenerateCommitments;
using node::VerifyLoadedChainstate;
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
</details>
thx, added a commit
87 | template <typename... Args> 88 | -bilingual_str format(const bilingual_str& fmt, const Args&... args) 89 | +bilingual_str format(util::BilingualFmt<sizeof...(Args)> fmt, const Args&... args) 90 | { 91 | - const auto translate_arg{[](const auto& arg, bool translated) -> const auto& { 92 | + const auto original_arg{[](const auto& arg) -> const auto& {
style nit: using internal function overloads seems a bit more readable to me, but up to you what you prefer
<details> <summary>git diff on fa93b05a4d</summary>
diff --git a/src/util/translation.h b/src/util/translation.h
index 27747a16f2..347bc5151d 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -81,29 +81,26 @@ consteval auto _(util::TranslatedLiteral str) { return str; }
/** Mark a bilingual_str as untranslated */
inline bilingual_str Untranslated(std::string original) { return {original, original}; }
+namespace internal {
+//! Helper functions for tfm::format to retrieve an original, untranslated version of an argument
+constexpr const auto& original_arg(const bilingual_str& arg) { return arg.original; }
+constexpr const auto& original_arg(const util::TranslatedLiteral& arg) { return arg.original; }
+template<typename T>
+constexpr const T& original_arg(const T& arg) { return arg; }
+
+//! Helper functions for tfm::format to retrieve a translated version of an argument
+constexpr const auto& translated_arg(const bilingual_str& arg) { return arg.translated; }
+template<typename T>
+constexpr const T& translated_arg(const T& arg) { return arg; }
+} // namespace internal
+
// Provide an overload of tinyformat::format for BilingualFmt format strings and bilingual_str or TranslatedLiteral args.
namespace tinyformat {
template <typename... Args>
bilingual_str format(util::BilingualFmt<sizeof...(Args)> fmt, const Args&... args)
{
- const auto original_arg{[](const auto& arg) -> const auto& {
- if constexpr (std::is_same_v<decltype(arg), const bilingual_str&>) {
- return arg.original;
- } else if constexpr (std::is_same_v<decltype(arg), const util::TranslatedLiteral&>) {
- return arg.original;
- } else {
- return arg;
- }
- }};
- const auto translated_arg{[](const auto& arg) -> const auto& {
- if constexpr (std::is_same_v<decltype(arg), const bilingual_str&>) {
- return arg.translated;
- } else {
- return arg;
- }
- }};
- return bilingual_str{tfm::format(fmt.original, original_arg(args)...),
- tfm::format(std::string{fmt.lit}, translated_arg(args)...)};
+ return bilingual_str{tfm::format(fmt.original, internal::original_arg(args)...),
+ tfm::format(std::string{fmt.lit}, internal::translated_arg(args)...)};
}
} // namespace tinyformat
</details>
leaving as-is for now
re-ACK fa51381790fe19e37e04a01a39cc94a00dcc441c
Just resolved conflict with #31616 since my last review.
Code review ACK fa51381790fe19e37e04a01a39cc94a00dcc441c. Just rebased and fixed minor conflict since last review.
ACK fa51381790fe19e37e04a01a39cc94a00dcc441c - replacing linters with compile time checks is a very nice win. The newly introduced TranslatedLiteral and BilingualFmt make for a nice interface, and RuntimeFormat preserves the ability to bypass compile-time checks in a clear and convenient way.
A couple of non-blocking nits (sorry, I thought I'd posted them already) that shouldn't hold up this PR.
The type was introduced in the previous commit.
The linter has many implementation bugs and missing features.
Also, it is completely redundant with FormatStringCheck, which
constructs from ConstevalFormatString or a runtime format string.
This brings the format types closer to the standard library types:
* FormatStringCheck corresponds to std::basic_format_string, with
compile-time checks done via ConstevalFormatString
* RuntimeFormat corresponds to std::runtime_format, with no compile-time
checks done.
Also, it documents where no compile-time checks are done.
re-ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e
No changes except for consistent re-use of TranslateFn, minor docstring change and un-inline-ing test translate function.
Code review ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant inline keyword