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

maflcko commented at 7:35 am on June 15, 2023: member

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

DrahtBot commented at 7:35 am on June 15, 2023: contributor

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.

DrahtBot added the label Refactoring on Jun 15, 2023

stickies-v commented at 10:49 am on June 15, 2023: contributor

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

maflcko commented at 11:00 am on June 15, 2023: member

LIFETIMEBOUND

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

stickies-v commented at 11:05 am on June 15, 2023: contributor

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 commented at 11:30 am on June 15, 2023: member

Concept ACK.

It helps in #27862.

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

+1

maflcko commented at 11:30 am on June 15, 2023: member

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:

 0diff --git a/src/util/translation.h b/src/util/translation.h
 1index f66f9de63a..8703741d9e 100644
 2--- a/src/util/translation.h
 3+++ b/src/util/translation.h
 4@@ -49,20 +49,16 @@ inline bilingual_str Untranslated(std::string original) { return {original, orig
 5 
 6 // Provide an overload of tinyformat::format which can take bilingual_str arguments.
 7 namespace tinyformat {
 8-inline const std::string& TranslateArg(const bilingual_str& arg, bool translated)
 9-{
10-    return translated ? arg.translated : arg.original;
11-}
12-
13-template <typename T>
14-const T& TranslateArg(const T& arg, bool translated)
15-{
16-    return arg;
17-}
18-
19 template <typename... Args>
20 bilingual_str format(const bilingual_str& fmt, const Args&... args)
21 {
22+    constexpr auto TranslateArg{[](const auto& arg, bool translated) {
23+        if constexpr (std::is_same_v<decltype(arg), const bilingual_str&>) {
24+            return translated ? arg.translated : arg.original;
25+        } else {
26+            return arg;
27+        }
28+    }};
29     return bilingual_str{format(fmt.original, TranslateArg(args, false)...),
30                          format(fmt.translated, TranslateArg(args, true)...)};
31 }

maflcko force-pushed on Jun 15, 2023

maflcko renamed this:
~~refactor: Avoid copy of bilingual_str when formatting~~
refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation
on Jun 15, 2023

maflcko commented at 12:57 pm on June 15, 2023: member

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

stickies-v approved

stickies-v commented at 1:25 pm on June 15, 2023: contributor

ACK faa857686c3693077a09fc571f63829b48e5d5b5

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?

in src/util/translation.h:55 in faa857686c outdated

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

ryanofsky commented at 1:48 pm on June 15, 2023:

In commit “refactor: Avoid copy of bilingual_str when formatting, Fix ADL violation” (faa857686c3693077a09fc571f63829b48e5d5b5)

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.

maflcko commented at 2:14 pm on June 15, 2023:

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.

ryanofsky approved

ryanofsky commented at 1:57 pm on June 15, 2023: contributor

Code review ACK faa857686c3693077a09fc571f63829b48e5d5b5

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.

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.

fa8ef7d138

maflcko force-pushed on Jun 15, 2023

maflcko commented at 2:29 pm on June 15, 2023: member

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 approved

ryanofsky commented at 2:33 pm on June 15, 2023: contributor

Code review ACK fa8ef7d138913d2f10482b0f1693ad94ce497f11. Looks great! Thanks for updating

DrahtBot requested review from stickies-v on Jun 15, 2023

hebasto approved

hebasto commented at 3:11 pm on June 15, 2023: member

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

achow101 commented at 6:22 pm on June 15, 2023: member

ACK fa8ef7d138913d2f10482b0f1693ad94ce497f11

achow101 merged this on Jun 15, 2023

achow101 closed this on Jun 15, 2023

maflcko deleted the branch on Jun 15, 2023

sidhujag referenced this in commit be1fb20010 on Jun 19, 2023

Fabcien referenced this in commit 58c73bcf68 on Mar 12, 2024

bitcoin locked this on Jun 14, 2024

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

Reviews