<table><tr><td style="">kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24261">View Revision</a></tr></table><br /><div><div><p>Thanks for review work, <a href="https://phabricator.kde.org/p/dhaumann/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dhaumann</a> :) Seems we have some differences though, so let's try to align knowledge:</p>
<p>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?</p>
<p>Also I have learned that using <tt style="background: #ebebeb; font-size: 13px;">for(T& t : tContainer)</tt> 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.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137432">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kedittoolbar.cpp:1566</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">No qAsConst? If done by intention, maybe keep the iterator version?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">By intention, as the elements of the vector are modified.<br />
Why would you keep the iterator version in that case?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137433">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kgesture.cpp:117</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">qAsConst?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Again, members of d->m_shape are changed (thus also the QPoint&)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137434">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kgesture.cpp:131</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><ol class="remarkup-list">
<li class="remarkup-list-item">QPoint &, or is sizeof QPoint==8?</li>
<li class="remarkup-list-item">Previously, the loop started at index 1.</li>
</ol></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why the sizeof==8 rule?</p>
<p style="padding: 0; margin: 8px;">Good catch about the 1, slipped me, will drop change here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137435">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kgesture.cpp:165</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">QPoint &</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">By value on purpose.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137436">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kmainwindow.cpp:349</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">qAsConst?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">dbusName and its chars ares modified here,</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137437">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kshortcutseditor.cpp:715</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Maybe make it a QVector. Or simply an initializer list.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Wanted to make it a plain array, but forgot.<br />
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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137438">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kswitchlanguagedialog_p.cpp:373</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">qAsConst</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">members of languagesList are changed.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137439">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ktoolbar.cpp:187</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Let's say values detaches here, then the changes value doesn't have any effect on the original one.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">values</tt> itself, so where do you see a non-original one?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24261#inline-137440">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ktoolbar.cpp:376</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">constexpr?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :) ).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R263 KXmlGui</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24261">https://phabricator.kde.org/D24261</a></div></div><br /><div><strong>To: </strong>kossebau, dfaure<br /><strong>Cc: </strong>dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>