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.
ACK b4e1480c02e845ab1df573d92acb814f62a172ea 🥃
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 %.
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:
0#include <QString>
1#include <QDebug>
2
3QString zoo() {
4 return QString{"zoo"};
5}
6
7int main()
8{
9 const auto foo = QString("bar") % QLatin1Char(' ') % zoo();
10 qDebug() << foo; // crash because QStringBuilder<T...> expression template will have references to dead temporaries
11}
“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 .
244@@ -244,7 +245,8 @@ static ProxySetting GetProxySetting(QSettings &settings, const QString &name)
245
246 static void SetProxySetting(QSettings &settings, const QString &name, const ProxySetting &ip_port)
247 {
248- settings.setValue(name, ip_port.ip + ":" + ip_port.port);
249+ QString proxy = ip_port.ip + QLatin1Char(':') + ip_port.port;
250+ settings.setValue(name, proxy);
QString{} temporary, or this is assumed to have better readability? If so, const could be added.
113@@ -114,7 +114,7 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
114 return (qint64)rec->nodeStats.nodeid;
115 case Address:
116 // prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
117- return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
118+ return QString::fromStdString((rec->nodeStats.fInbound ? "↓ " : "↑ ") + rec->nodeStats.addrName);
0qt/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'
1 return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
2 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
45 return tr("conflicted with a transaction with %1 confirmations").arg(-nDepth);
46- else if (nDepth == 0)
47- return tr("0/unconfirmed, %1").arg((inMempool ? tr("in memory pool") : tr("not in memory pool"))) + (status.is_abandoned ? ", "+tr("abandoned") : "");
48- else if (nDepth < 6)
49+ } else if (nDepth == 0) {
50+ QString abandoned;
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.
The defined QT_USE_QSTRINGBUILDER macro means using the QStringBuilder
for efficient string concatenation in all Qt code by default.
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.
ACK a02c970eb001b456d74ddc30750fe8b55348ddac
Tested most of the strings on Qt 5.15.2 macOS 11.3
Code review ACK a02c970eb001b456d74ddc30750fe8b55348ddac
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.
