D24261: Modernize code: use range-based for loop in more places

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Thanks for review work, @dhaumann  :) Seems we have some differences though, so let's try to align knowledge:
  
  When it comes to by-value or by reference of the loop range element value, I learned that similar rules as for arguments are valid: small size in bytes & trivial copy constructor -> value, otherwise reference. QPoint thus would be candidate for by-value. Would I need to improve my knowledge?
  
  Also I have learned that using `for(T& t : tContainer)` is fine for iterating through a container to change its values, and preferred over its short expression as well. Used to in other places, so surprised by the comments.

INLINE COMMENTS

> dhaumann wrote in kedittoolbar.cpp:1566
> No qAsConst? If done by intention, maybe keep the iterator version?

By intention, as the elements of the vector are modified.
Why would you keep the iterator version in that case?

> dhaumann wrote in kgesture.cpp:117
> qAsConst?

Again, members of d->m_shape are changed (thus also the QPoint&)

> dhaumann wrote in kgesture.cpp:131
> 1. QPoint &, or is sizeof QPoint==8?
> 2. Previously, the loop started at index 1.

Why the sizeof==8 rule?

Good catch about the 1, slipped me, will drop change here.

> dhaumann wrote in kgesture.cpp:165
> QPoint &

By value on purpose.

> dhaumann wrote in kmainwindow.cpp:349
> qAsConst?

dbusName and its chars ares modified here,

> dhaumann wrote in kshortcutseditor.cpp:715
> Maybe make it a QVector. Or simply an initializer list.

Wanted to make it a plain array, but forgot.
Still curious about the idea with the uses of initializer list as "list", container so far I have not seen it elsewhere and wonder what the effect is.

> dhaumann wrote in kswitchlanguagedialog_p.cpp:373
> qAsConst

members of languagesList are changed.

> dhaumann wrote in ktoolbar.cpp:187
> Let's say values detaches here, then the changes value doesn't have any effect on the original one.

values detaches as before (where by use of non-const operator[]),now by internal use of begin()/end(), but this is fine, no? The "original" one is the class member `values` itself, so where do you see a non-original one?

> dhaumann wrote in ktoolbar.cpp:376
> constexpr?

Not directly related/needed to the actual change. Going for const arrays and making them constexpr I would do in a separate dedicated commit (with my having-had-to-read-lots-of.commit-history hat on :) ).

REPOSITORY
  R263 KXmlGui

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

To: kossebau, dfaure
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190928/bf4c3f27/attachment-0001.html>