refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation #27892

maflcko · 2023-06-15T07:35:49Z

This refactor shouldn't change behavior, but may fix compile errors such as #27862 (comment)

DrahtBot · 2023-06-15T07:35:51Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, hebasto, achow101
Stale 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.

stickies-v

Approach ACK. Perhaps worth annotating both arg parameters with LIFETIMEBOUND?

maflcko · 2023-06-15T11:00:34Z

LIFETIMEBOUND

May give it a try if I have to re-touch.

stickies-v · 2023-06-15T11:05:58Z

It's a pretty trivial change? And if it doesn't work, wouldn't that indicate the change is/may be unsafe and we probably want to rethink this PR?

hebasto · 2023-06-15T11:30:10Z

Concept ACK.

It helps in #27862.

Approach ACK. Perhaps worth annotating both arg parameters with LIFETIMEBOUND?

+1

maflcko · 2023-06-15T11:30:41Z

It's a pretty trivial change? And if it doesn't work, wouldn't that indicate the change is/may be unsafe and we probably want to rethink this PR?

It requires pulling in a new header and using the keyword in the tinyformat namespace, which I guess is ok. If the change was unsafe, it would have already been unsafe in master because T& is already a lifetime-bound reference. Given that the function is only used in one statement, it should be easier and faster to just check by reading the one line instead of reading the keyword and then thinking about the keyword.

If you want, I can change the code to make the helper private to the only statement scope that uses it:

diff --git a/src/util/translation.h b/src/util/translation.h
index f66f9de63a..8703741d9e 100644
--- a/src/util/translation.h
+++ b/src/util/translation.h
@@ -49,20 +49,16 @@ inline bilingual_str Untranslated(std::string original) { return {original, orig
 
 // Provide an overload of tinyformat::format which can take bilingual_str arguments.
 namespace tinyformat {
-inline const std::string& TranslateArg(const bilingual_str& arg, bool translated)
-{
-    return translated ? arg.translated : arg.original;
-}
-
-template <typename T>
-const T& TranslateArg(const T& arg, bool translated)
-{
-    return arg;
-}
-
 template <typename... Args>
 bilingual_str format(const bilingual_str& fmt, const Args&... args)
 {
+    constexpr auto TranslateArg{[](const auto& arg, bool translated) {
+        if constexpr (std::is_same_v<decltype(arg), const bilingual_str&>) {
+            return translated ? arg.translated : arg.original;
+        } else {
+            return arg;
+        }
+    }};
     return bilingual_str{format(fmt.original, TranslateArg(args, false)...),
                          format(fmt.translated, TranslateArg(args, true)...)};
 }

maflcko · 2023-06-15T12:57:53Z

Force pushed to use the lambda approach. Also, fixed the ADL violation found by @hebasto in #27862 (comment)

stickies-v

ACK faa8576

Lambda approach is elegant. I would've been okay with a simple LIFETIMEBOUND attribute but this is probably better, thanks for reworking it.

it would have already been unsafe in master because T& is already a lifetime-bound reference.

Yes, but I think that doesn't hold for what was the untemplated TranslateArg? It doesn't really matter here because format returns a copy anyway, but it would make new callsites of std::string TranslateArg(const bilingual_str& arg, bool translated) potentially unsafe?

ryanofsky

Code review ACK faa8576

Thanks for the fix. The code looks safe and should fix the namespace problem, but I had some questions about how it is working internally, see below. They aren't important, just asking to improve my understanding.

ryanofsky · 2023-06-15T13:48:06Z

src/util/translation.h

 template <typename... Args>
 bilingual_str format(const bilingual_str& fmt, const Args&... args)
 {
-    return bilingual_str{format(fmt.original, TranslateArg(args, false)...),
-                         format(fmt.translated, TranslateArg(args, true)...)};
+    constexpr auto TranslateArg{[](const auto& arg, bool translated) {


In commit "refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation" (faa8576)

Commit messsage says this is more efficient because TranslateArg lambda returns a reference. But how do you know it actually does return a reference? I'm not that familiar with the rules for deduced return values, so you probably know more here, but if the default return type is -> auto I would expect it to return by value not reference. Maybe can add -> const auto& return type to force returning a reference or just be clear that it is supposed to be a reference?

Also, should type of lambda variable be const auto instead of constexpr auto? Again I could be wrong but const auto suggests to me lambda does not have mutable state, which is true, while constexpr auto suggests lambda will be called by compile time code, which could be true, but is not true right now because format function is not constexpr.

Also maybe would rename TranslateArg to translate_arg since it is a local variable.

Good point. The return type is auto, not (as I assumed decltype(auto)). See also #20495

I guess I'll need to use -> decltype(auto) or -> const auto&, as you suggest.

The return type of TranslateArg is std::string, which creates a copy. Fix this by moving everything into a lambda that takes a reference and returns a reference. Also, the format function is called without specifying the namespace it lives in. Fix this by specifying the namespace. See also: https://github.com/bitcoin/bitcoin/blob/7a59865793cd710d7d6650a6106ca4e790ced5d3/doc/developer-notes.md#L117-L137.

maflcko · 2023-06-15T14:29:53Z

LIFETIMEBOUND

To clarify, it should be possible to add this attribute to a lambda, but I still don't think it is useful here.

ryanofsky

Code review ACK fa8ef7d. Looks great! Thanks for updating

hebasto

ACK fa8ef7d, I have reviewed the code and it looks OK.

achow101 · 2023-06-15T18:22:12Z

ACK fa8ef7d

…tting, Fix ADL violation fa8ef7d refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation (MarcoFalke) Pull request description: This refactor shouldn't change behavior, but may fix compile errors such as bitcoin#27862 (comment) ACKs for top commit: achow101: ACK fa8ef7d ryanofsky: Code review ACK fa8ef7d. Looks great! Thanks for updating hebasto: ACK fa8ef7d, I have reviewed the code and it looks OK. Tree-SHA512: 903019962f27b5432b8e3af052b472238ef68d3ee165148c9d2232bf290309075f9f17d8d06c9b5c7fddb89c1a9c3a4c09c6310af01e8561adc0244a30db0857

Summary: Previous bilingual_str tinyformat::format accepted bilingual format strings, but not bilingual arguments. Extend it to accept both. This is useful when embedding one translated string inside another translated string, for example: `strprintf(_("Error: %s"), message)` which would fail previously if `message` was a bilingual_str. This is a partial backport of [[bitcoin/bitcoin#27150 | core#27150]] bitcoin/bitcoin@3db2874 ---- refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation The return type of TranslateArg is std::string, which creates a copy. Fix this by moving everything into a lambda that takes a reference and returns a reference. Also, the format function is called without specifying the namespace it lives in. Fix this by specifying the namespace. See also: https://github.com/bitcoin/bitcoin/blob/7a59865793cd710d7d6650a6106ca4e790ced5d3/doc/developer-notes.md#L117-L137. This is a backport of [[bitcoin/bitcoin#27892 | core#27892]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14993

DrahtBot added the Refactoring label Jun 15, 2023

maflcko mentioned this pull request Jun 15, 2023

validation: Stricter assumeutxo error handling when renaming chainstates #27862

Merged

stickies-v reviewed Jun 15, 2023

View reviewed changes

maflcko force-pushed the 2306-translate-copy- branch from fa0e607 to faa8576 Compare June 15, 2023 12:56

maflcko changed the title ~~refactor: Avoid copy of bilingual_str when formatting~~ refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation Jun 15, 2023

stickies-v approved these changes Jun 15, 2023

View reviewed changes

ryanofsky approved these changes Jun 15, 2023

View reviewed changes

maflcko force-pushed the 2306-translate-copy- branch from faa8576 to fa8ef7d Compare June 15, 2023 14:22

ryanofsky approved these changes Jun 15, 2023

View reviewed changes

DrahtBot requested a review from stickies-v June 15, 2023 14:33

hebasto approved these changes Jun 15, 2023

View reviewed changes

achow101 merged commit 5b8e077 into bitcoin:master Jun 15, 2023

maflcko deleted the 2306-translate-copy- branch June 15, 2023 18:33

bitcoin locked and limited conversation to collaborators Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation #27892

refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation #27892

Uh oh!

maflcko commented Jun 15, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Jun 15, 2023 •

edited

Loading

Uh oh!

stickies-v left a comment

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

stickies-v commented Jun 15, 2023

Uh oh!

hebasto commented Jun 15, 2023

Uh oh!

maflcko commented Jun 15, 2023 •

edited

Loading

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

stickies-v left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jun 15, 2023

Uh oh!

maflcko Jun 15, 2023

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

ryanofsky left a comment

Uh oh!

hebasto left a comment

Uh oh!

achow101 commented Jun 15, 2023

Uh oh!

Uh oh!

refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation #27892

refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation #27892

Uh oh!

Conversation

maflcko commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

stickies-v commented Jun 15, 2023

Uh oh!

hebasto commented Jun 15, 2023

Uh oh!

maflcko commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 15, 2023

Uh oh!

Uh oh!

maflcko commented Jun 15, 2023 •

edited

Loading

DrahtBot commented Jun 15, 2023 •

edited

Loading

maflcko commented Jun 15, 2023 •

edited

Loading