D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
This revision was automatically updated to reflect the committed changes.
Closed by commit R278:1081a6b284ba: Port away from Qts foreach (authored 
by kossebau).

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23839?vs=65768=65782

REVISION DETAIL
  https://phabricator.kde.org/D23839

AFFECTED FILES
  autotests/kwindowsystem_threadtest.cpp
  src/platforms/xcb/kwindoweffects.cpp
  src/platforms/xcb/kwindowsystem.cpp
  src/platforms/xcb/netwm.cpp
  src/pluginwrapper.cpp

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Vlad Zagorodniy
zzag accepted this revision.
zzag added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R278 KWindowSystem

BRANCH
  portfromforeach

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
kossebau added a comment.


  @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? :)

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
kossebau added inline comments.

INLINE COMMENTS

> zzag wrote in kwindowsystem_threadtest.cpp:61
> I asked that because `for (const Type  : collection) {` is more common. 
> I know that const ref doesn't have any advantages here.

(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.

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
kossebau added a comment.


  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).

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Vlad Zagorodniy
zzag added inline comments.

INLINE COMMENTS

> zzag wrote in kwindowsystem_threadtest.cpp:61
> I asked that because `for (const Type  : collection) {` is more common. 
> I know that const ref doesn't have any advantages here.

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

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Vlad Zagorodniy
zzag added inline comments.

INLINE COMMENTS

> kossebau wrote in 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?

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

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
kossebau added a comment.


  In D23839#528788 , @zzag wrote:
  
  > Did you use a script to create this patch?
  
  
  Nope, manually done.

INLINE COMMENTS

> zzag wrote in kwindowsystem_threadtest.cpp:61
> Capture the value by const ref please.

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?

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Vlad Zagorodniy
zzag added a comment.


  Did you use a script to create this patch?

INLINE COMMENTS

> kwindowsystem_threadtest.cpp:61
>  const QList windows = KWindowSystem::stackingOrder();
> -foreach (auto wid, windows) {
> +for (auto wid : windows) {
>  KWindowInfo info(wid, NET::WMVisibleName);

Capture the value by const ref please.

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Albert Astals Cid
aacid added a comment.


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

REPOSITORY
  R278 KWindowSystem

REVISION DETAIL
  https://phabricator.kde.org/D23839

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


D23839: Port away from Qt's foreach

2019-09-10 Thread Friedrich W. H. Kossebau
kossebau created this revision.
kossebau added reviewers: KWin, zzag.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REPOSITORY
  R278 KWindowSystem

BRANCH
  portfromforeach

REVISION DETAIL
  https://phabricator.kde.org/D23839

AFFECTED FILES
  autotests/kwindowsystem_threadtest.cpp
  src/platforms/xcb/kwindoweffects.cpp
  src/platforms/xcb/kwindowsystem.cpp
  src/platforms/xcb/netwm.cpp
  src/pluginwrapper.cpp

To: kossebau, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns