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
