qt: Optimize string concatenation by default

[hebasto]

From Qt docs:

... multiple uses of the [QString] '+' operator usually means multiple memory allocations. When concatenating n substrings, where n > 2, there can be as many as n - 1 calls to the memory allocator.

With this PR

... the '+' will automatically be performed as the QStringBuilder '%' everywhere.

The change in the src/Makefile.qt.include file does not justify submitting this PR into the main repo, IMHO.

[jarolrod]

ACK b4e1480 🥃

This simplifies and reduces the mental burden of having to think about "am I concatenating a string in the most efficient way possible?"

I could not find any other occurrences of including QStringBuilder or occurrences of concatenating string with %.

[hebasto]

@Talkless

Could you look into this PR?

[Talkless]

It's true that QtCreator, and Qt itself is built with that flag, but we might want to discuss if we want that "implicit magic". There is some caveat with this kind of code:

#include <QString>
#include <QDebug>

QString zoo() {
    return QString{"zoo"};
}

int main()
{
    const auto foo = QString("bar") % QLatin1Char(' ') % zoo();
    qDebug() << foo; // crash because QStringBuilder<T...> expression template will have references to dead temporaries
}

"maybe" it is possible to write similar code and "sometimes" not crash, IDK. Not sure if this is dangerous in any new code, i.e. if it will be missed by reviewers, if it will pass without "sometimes" crashing, etc. Although, we are talking about GUI code, so.. not sure.

I personally still use % for "explicitness", but as @jarolrod said, it removes burned of thinking about inefficient concatenation & stuff... And less annoying nit comments from me :D .

[Talkless]

Concept ACK

[Talkless]

What is motive for this change?

[hebasto]

qt/peertablemodel.cpp:117:20: error: no viable conversion from returned value of type 'QStringBuilder<typename QConcatenable<QString>::type, typename QConcatenable<QString>::type>' (aka 'QStringBuilder<QString, QString>') to function return type 'QVariant'
            return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[hebasto]

@Talkless

It's true that QtCreator, and Qt itself is built with that flag, but we might want to discuss if we want that "implicit magic".

You are right! Discussion is a good mean to reach (rough) consensus about suggested changes.

There is some caveat with this kind of code:

You code snippet could be fixed by s/auto/QString/, and such caveats could be documented in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code.

[hebasto]

Updated b4e1480 -> a02c970 (pr313.02 -> pr313.03, diff):

• addressed @Talkless's comments

[Talkless]

You code snippet could be fixed by s/auto/QString/

Yes, but that's the "danger", as one needs to know these kinda caveats. Yes, docs could help.

[Talkless]

utACK a02c970, built successfully on Debian Sid with Qt 5.15.2, but did not check if any displayed strings are "wrong" after refactoring.

[jarolrod]

ACK a02c970

Tested most of the strings on Qt 5.15.2 macOS 11.3

[laanwj]

Code review ACK a02c970

This simplifies and reduces the mental burden of having to think about "am I concatenating a string in the most efficient way possible?"

I don't think there is any place in bitcoin where concatenating Qt strings is actually a bottleneck, but agree with this perspective.

It's true that QtCreator, and Qt itself is built with that flag,

That's reassuring also in the sense that it probably won't just go away.

[jonatack]

Approach ACK, this looks like a good idea indeed.