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

[André Hartmann]

Hi all,

I fully agree, Olivier.

Looking at https://docs.kdab.com/analysis/qtcreator/clazy.html gives 
currently 223 potential detaching containers within range-based for, and 
qtbase alone has 46 (https://docs.kdab.com/analysis/qt5/clazy.html).

Even if there may be some false positives, who is going to check and fix 
them all? If we don't manage to use the range-based for correctly (for 
Qt-containers), why should we force the user to port away from foreach?

Just my two cent.

Best regards,
André

Am 29.10.18 um 12:43 schrieb 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)
>