[Development] qMoveToConst helper for rvalue references to movable Qt containers?

[Olivier Goffart]

On 10/28/18 8:17 PM, Thiago Macieira wrote:
> On Sunday, 28 October 2018 11:49:08 PDT Olivier Goffart wrote:
>> It is a bit ironic that one reason given to deprecate Q_FOREACH is that it
>> may copy the container in some cases, while the alternative has the same
>> problem in much more common cases. (It is my impression that implicitly
>> shared container such as QList/QVector are by far much more used than the
>> one that are not within a typical Qt code base)
> 
> There are two problems with Q_FOREACH:
> 
> 1) it will copy containers. For Qt containers, that's rather cheap (two atomic
> refcount operations), but it's not free. And for Standard Library containers,
> that is likely very expensive.

But using for(:) with a Qt container will cause a detach in the most common 
case, so I'd say is even worse.
Which is why i'm saying using this reason is a bit ironic.

> 
> 2) it's implemented by way of a for loop inside a for loop, which is known to
> throw optimisers out, generating slightly worse code

I would consider that the missed optimization is quite small, if not negligible.
And it can be solved in C++17:
https://codereview.qt-project.org/243984/

> Our rule already was: Don't use foreach in Qt code. (it was fine in examples)
> 
> Using iterators and now using the ranged for may need more typing, but
> produces optimal code.
> 
>> What could be done is to only deprecate partial specialization of
>> qMakeForeachContainer for QVarLenghtArray and the standard containers.
>> Or for containers that do not have a 'detach' function.
>> That way, users would get a warning for the problematic containers, but
>> would continue to work just fine with with the containers most Qt developer
>> use.
> 
> I disagree. The optimisation problem is not solved.

I'm ok with discouraging Q_FOREACH, but deprecating such a function need more 
thinking.
Deprecating means you will force user to port all their codebase away from it, 
which is a huge work. If the rationale is just that they will save a couple of 
atomic operations, i do not think it is worth it.

Deprecating it only for non-shared container seems more logical, since we then 
warn only when there is actually a problem.

And for the Qt shared container case, using foreach is less typing, but also 
less error prone. (do i have to use qAsConst? or make a copy? or even return a 
const object which even lead to more problems)

-- 
Olivier

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org