Port most cases of Q_FOREACH to the new for loop.
AbandonedPublic

Authored by stikonas on Jun 3 2017, 5:55 PM.

Details

Reviewers
graesslin
Test Plan

Tested kwin_x11 a bit, seems to work.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
stikonas created this revision.Jun 3 2017, 5:55 PM
Restricted Application added subscribers: KWin, kwin. · View Herald TranscriptJun 3 2017, 5:55 PM

The hard part of moving to the new for syntax isn't changing the code.

The hard part is spotting where there's a behavioural difference that could come from any code inside the loop (including potentially called functions) between for and foreach.

composite.cpp
725–727

No!!!!!

This is exactly why a blind port of all for loops is very dangerous.

foreach takes an implicit copy of the list when it starts.

windows.removeAll(t) would remove from the original list, but the one we're iterrating over gets detached and therefore unaffected.

This new code will either miss some windows or crash violently, especially as you've const casted it.

stikonas abandoned this revision.Jun 5 2017, 11:56 PM
apol added a subscriber: apol.Jun 6 2017, 9:13 AM

The hard part of moving to the new for syntax isn't changing the code.

The hard part is spotting where there's a behavioural difference that could come from any code inside the loop (including potentially called functions) between for and foreach.

In fairness, I don't see how foreach will do much better with removeAll in the middle, this one should be ported to iterators anyway.

In fairness, I don't see how foreach will do much better with removeAll in the middle

Then please don't port any foreach loops till you do.

Qt automatically takes a copy of the container when it enters a foreach loop. If you modify the container as you are iterating, that won't affect the loop. (If you do not modify the container, the copy still takes place, but thanks to implicit sharing copying a container is very fast.)

In D6077#114439, @apol wrote:

In fairness, I don't see how foreach will do much better with removeAll in the middle, this one should be ported to iterators anyway.

Q_FOREACH makes an implicitly shared copy. The moment you do sth. destructive on the original list, the iterated list becomes a deep copy and remains unmodified. Operating on iterators is usually™ more performant (you spare the deep copy but need to maintain the iterator consistency) but at least this on is a very rare occasion for an actual requirement to alter the list.

graesslin edited edge metadata.Jun 10 2017, 1:02 PM

Just for sale of completeness: I'm against changes like this one as we gain nothing but introduce a huge risk of breakage.