[Development] Q_FOREACH, again

[Marc Mutz]

Hi,

I've uploaded a patch series that makes QT_NO_FOREACH the default and 
whitelists any existing uses of Q_FOREACH/foreach as can be seen in the 
culminating patch of the series: 
https://codereview.qt-project.org/c/qt/qtbase/+/495115

The intention of this patch series is not to fix all uses, the intention 
is to change the default from allowing Q_FOREACH to disallowing 
Q_FOREACH in new Qt (library|plugin|tool|test) code.

It does _not_ have any influence on user code.

It has, however, sparked a flurry of commits that attempt to fix 
remaining uses, and I'd like to give everyone that tries a heads up:

For your own sanity, and those of your reviewers, remember that the port 
is _not easy_. Details are in https://www.kdab.com/goodbye-q_foreach/, 
but here's the TL;DR: in current terminology.

There's _one_ situation which I call "trivial": When the loop is over a 
_const_ local container (local _value_, const-references or data members 
do _not_ count). For me, it's ok to say in the commit message that these 
are of the trivial kind (but _say_ it in the commit message), it's 
trivially(!) verified by a reviewer that it's correct.

There's also what I call a "simple" case: where you iterate over a local 
container that's _not_ const, but where the loop body _clearly_ doesn't 
modify the container. That must be _clearly_ visible. If it calls an 
out-of-line function, that should already spark an explanation in the 
commit message (unless it's something every Qt developer should know, 
like QString::toUtf8() doesn't call unknown code).

If it's not one the two, you should do two things:

- port only one loop (or at most a group of loops over the same
   variable) per commit. This allows us to quickly revert such a change
   if it goes south, or even find it, with git-bisect, in the first
   place.
- _proove_ that the loop body cannot modify the container. Write the
   proof in the _commit message_. Watch out for signal emissions,
   callbacks, events being sent, etc under the hood that can cause
   unknown code to be called. If you find something like this, you _must_
   take a copy and iterate over that, or use an indexed loop. Unless the
   modification is obvious, add a _code comment_ that explains why we
   take a copy/use an indexed loop.

If you can't proove it, don't do the change, or take a copy and say "// 
### needed?". Just don't do that _everywhere_.

My patch series takes the pressure of porting away from Q_FOREACH away. 
We don't need to do it now, and when we can depend on C++20, any 
Q_FOREACH use can be trivially ported to ranged-for-with-initializer:

    #define Q_FOREACH(x, y) for (const auto _c = y; x : _c)

So we can remove all remaining ones in an automated fashion then.

You will, of course, find reviewers who will approve such ports even 
though the proof is missing. Given that we no longer need to port away, 
that would be foolish, but I fully expect it to happen. Just make sure _you_
are not that fool.

Thanks,
Marc

-- 
Marc Mutz <marc.mutz at qt.io>
Principal Software Engineer

The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io

Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B