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.m...@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 -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
