In commit "refactor: Mark run-time Untranslated() c_str with std::string" (fa213aa)

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("..."), ...) with Untranslated(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 (comment)

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:

• Untranslated function is simpler. Has a single definition, and is not overloaded to return different things depending on whether it is passed a std:::string or a c string.
• "Original" struct no longer requires a template parameter, it can be a simple struct.
• This commit can be dropped, the clang tidy commit can be dropped, and there's no need to convert add manual conversions from Untranslated to bilingual_str next commit.

Branch is https://github.com/ryanofsky/bitcoin/commits/pr/tfmt, commits are:

• ab4e567 refactor: Don't embed translated strings in untranslated strings.
• d5a8e2b scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf)
• 5e0b4f9 refactor: Check wallet format strings at compile time
• 6183767 refactor: Pick translated string after format
• 71d3826 move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN
• 86ea584 refactor: Delay translation of Untranslated() or _() literals
• 08bab04 refactor: Check translatable format strings at compile-time

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 (comment)

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 ab4e567 wasn't doing an a good job of describing changes there. I think most of the 54 cases changed in d5a8e2b 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 (comment)

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 08bab04 from #31061 (comment) into this pull (rebased)

in aaaa4fb: 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 (comment)

in aaaa4fb: 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.

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

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 _>.

re: #31061 (comment)

Would be reasonable to change this but no more change is actually required here with my suggested branch.

re: #31061 (comment)

Simplified this further in 6fb3965 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 (comment)

The hunk isn't needed anymore, so I dropped it.

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?

re: #31061 (comment)

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 ab4e567 wasn't doing an a good job of describing changes there. I think most of the 54 cases changed in d5a8e2b 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.

In commit "refactor: Check translatable format strings at compile-time" (fa94825)

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)])");
 }

nit: Arguably tinyformat is more external than util/string.h, but our copy lives with our code so it's certainly not clear-cut.

In commit "refactor: Check translatable format strings at compile-time" (fa0b20a)

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