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.
seems good to me, but let someone of the people that actually know the code to the +2
Did you use a script to create this patch?
autotests/kwindowsystem_threadtest.cpp | ||
---|---|---|
61 | Capture the value by const ref please. |
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? |
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. |
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).
autotests/kwindowsystem_threadtest.cpp | ||
---|---|---|
61 | (Gah, phab ate this comment before, rewriting) |
@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? :)
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.