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)
_______________________________________________
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
