Port away from Qt's foreach
ClosedPublic

Authored by kossebau on Sep 10 2019, 3:01 PM.

Details

Reviewers
zzag
Group Reviewers
KWin
Commits
R278:1081a6b284ba: Port away from Qt's foreach

Diff Detail

Repository
R278 KWindowSystem
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Sep 10 2019, 3:01 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 10 2019, 3:01 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Sep 10 2019, 3:01 PM
aacid added a subscriber: aacid.Sep 10 2019, 3:10 PM

seems good to me, but let someone of the people that actually know the code to the +2

zzag added a comment.Sep 10 2019, 3:24 PM

Did you use a script to create this patch?

autotests/kwindowsystem_threadtest.cpp
61

Capture the value by const ref please.

In D23839#528788, @zzag wrote:

Did you use a script to create this patch?

Nope, manually done.

autotests/kwindowsystem_threadtest.cpp
61

Would a const ref make sense here, given the type nature of WId which boils down to a integer matching the byte size of a pointer?

zzag added inline comments.Sep 10 2019, 3:41 PM
autotests/kwindowsystem_threadtest.cpp
61

I asked that because for (const Type &item : collection) { is more common. I know that const ref doesn't have any advantages here.

zzag added inline comments.Sep 10 2019, 3:42 PM
autotests/kwindowsystem_threadtest.cpp
61

In either case, this change is good to go. However, it would be great to capture values by const ref.

Thanks for review, Albert & Vlad.

BTW; still one foreach left in macOS branch of code, which I could not test-drive, so did not change (also touching internals that I could not quickly understand if there is a chance to conflicting container changes in the call chains from the loop).

kossebau added inline comments.Sep 10 2019, 3:58 PM
autotests/kwindowsystem_threadtest.cpp
61

(Gah, phab ate this comment before, rewriting)
Getting the item by value though is also common, for types which are trivially copyable and fit in cpu registers on usual hardware. Which is some µ-opt that might make sense to get used to as code pattern by using it as default in such cases, to approach what they call "premature pessimation".
Your code though and not performance crititcal, so doing as you prefer.

@zzag Actually, while you commented on that one loop only, the same would be valid also for other loops touched in the patch. So, do you want const ref with all of them? As you can see by the existing code, it also already used values, not const ref, surely also for the reasons I gave. So, how much do you prefer const ref just for reading patterns? Shall I change also all the orher places? Or is there a chance you can be won for value types in for loops where of runtime advantage? :)

zzag accepted this revision.Sep 10 2019, 4:39 PM

I tend to leave only one comment about troubling issue/problem and expect that the author of a patch will address all other occurrences of the issue/problem.

However, let's get this change in. Perhaps I'm too picky about const refs.

This revision is now accepted and ready to land.Sep 10 2019, 4:39 PM
This revision was automatically updated to reflect the committed changes.