[Development] Use of Standard Library containers in Qt source code

[Mark Gaiser]

On Fri, Jul 1, 2016 at 8:36 PM, Thiago Macieira <thiago.macieira at intel.com>
wrote:

> Premises not under discussion:
>
>         Qt source code is product and meant to be read by our users
>         Qt source code must be clean and readable
>
> The above is not up for debate.
>
> For some time now, we've had a flurry of changes to Qt source code that
> uses
> the Standard Library's containers and algorithms, in the name of
> performance
> and often code size gains.
>
> I'm not disputing that there is a gain. But I am wondering about the
> trade-off
> we're making with regards to readability. For example, I was just reviewing
> this code:
>
>     if (d->currentReadChannel >= d->readHeaders.size()
>         || d->readHeaders[d->currentReadChannel].empty()) {
>         Q_ASSERT(d->buffer.isEmpty());
>
> The use of the Standard Library member "empty" is highly confusing at first
> sight because it does not follow the Qt naming guidelines. It's even more
> confusing because the next line has "isEmpty". When I read this code, I
> had to
> wonder if that "empty" was a verb in the imperative, meaning the author was
> trying to remove all elements from the container.
>
> I had to look up the definition of readHeaders in the review and note that
> it
> was a std::deque, not a Qt container.
>
> What do we do?
>
> Option 1:
> Not use Standard Library containers, just use the Qt containers as they
> exist.
>
> Option 2:
> Create new Qt containers to have the same complexity as Standard Library
> containers, but following the Qt naming conventions. Possibly with implicit
> sharing.
>
> Option 3:
> Create Qt API wrappers for those containers like std::deque, adding only a
> few
> inline functions to match the Qt-style API where the Standard Library API
> deviates. Examples are:
>         empty           ->      isEmpty
>         push_back       ->      append
>         front           ->      first
>         pop_front       ->      takeFirst
>         cbegin          ->      constBegin
>         cfind           ->      constFind
>
> I don't get why you would be confused by - for instance - the empty method.
STL has that method, but so does Qt [1] apparently in an attempt to be
compatible with the STL API.

Even if you don't want to learn or know how the STL API looks like, it
should not matter since the very same members also apply on Qt containers
with the same .
This is the case for all your examples in Option 3.

Imho it would be nice - when using Qt and STL containers in the same
statement - to use common functions that mean the same in both API's. Thus
in your example both should have been empty() calls, even on the Qt
container to be consistent. But that's just my opinion :)

[1] http://doc.qt.io/qt-5/qlist.html#empty
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20160702/e6f0ed57/attachment.html>