Hi Lars,
Let me pick important points one per email here:
On 2020-05-12 09:49, Lars Knoll wrote:
[...]
For String related classes:
* All methods not taking ownership of the passed arguments take a
QStringView
Almost (see below).
* If the method stores a pointer to the passed data it should take a
QString to not surprise users.
... in addition ... (though see below).
Exceptions can be done where it makes
sense, but then the method naming has to give clear indications that
this happens (like e.g. fromRawData())
* Return a QString in QString itself and when doing conversions,
return QStringView from QStringView
* No QStringRef!
* QLatin1String for backwards compatibility, can be disabled with a
macro (similar to QT_NO_CAST_FROM_ASCII)
* Remove or deprecate overloads taking a (const Char *, length) pair.
Replace them with QStringView
+1 to the above
Most other classes:
* Only take and return QString
This is wrong. By taking a QString in the API of non-string-related
APIs, you expose QString as an implementation detail of the class. Think
QRegion, which, before I added begin()/end(), had a SSO-like internal
container (1 QRect or QVector<QRect>) which forced most users to code
around the owning-container API like this:
if (r.rectCount() == 1) {
use(r.boundingRect());
} else {
const auto rects = r.rects();
for (const QRect &rect : rects)
use(rect);
}
If rects() had been a view (today, we'd use gsl/std::span<const QRect>),
all users could just do
for (const QRect &rect : r.rects())
use(rect);
This is objectively a better API, for two reasons: a) the user doesn't
need to care about some weird idiosyncrasies of the class to avoid
performance penalties and b) the class author is now free to extend the
SSO buffer from one to, say, four, without changing the API, not even
those affected by Hyrum.
It _seems_ your solution is to fold views into owning containers, and
while that may seem to work, it's dangerous:
Assume QRegion::rects() returned a QVector-acting-as-view. Then this
would silently fail:
QRegion region();
for (const QRect &rect : region().rects())
use(rect);
because, clearly, QVector is an owning container, so we don't care that
the QRegion went out of scope. Whereas with a view, it will be
immediately obvious (to a tool like Clazy, at least) that this can't
work.
So, I'd argue for the complete opposite here: we would increase
encapsulation of our APIs if they stopped trafficking in owning
container types. I call this the NOI pattern (Non-Owning Interface). By
not having to serve QString everywhere, we'll be much more free to use
alternative storage types in the implementation (e.g.
QVarLengthArray<char16_t>, or - the horror! - std::pmr::u16string).
Handling-wise, QStringView makes all these choices equal, so the
implementation of a class can use whatever is objectively optimal
instead of being bound to QString.
AsidE: If you think that CoW is still a thing today: no. SSO is a thing
these days, and it seems that QString will not have it in Qt 6, either.
NOI favours SSO, QString-everywhere cements the naïve CoW world of the
1990s for yet another decade.
Learn from QRegion!
I have spoken on many conferences (at least QtWS, Meeting C++, emBO++)
on this, if anyone wants to learn more.
Thanks,
Marc
_______________________________________________
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
