ahmadsamir added inline comments. INLINE COMMENTS
> kossebau wrote in kkeysequencewidget.cpp:127 > Is it? > This patch as is now has 3 changes: > > - change a foreach loop over extracted key list of a QHash to then also > access values (2 discouraged things) to iterator-based for loop over the QHash > - change a foreach loop over a QList to a range-based for loop over the QList > - cache the result of toString() outside of the inner loop > > Each of those is an independent change. The first two both fall into the > domain of "port away from foreach". The last one, as could be also seen in > the first version of this patch, has nothing to do with them, `seq` was a > const reference before and has been after (well, then represented/replaced > by`it.key()`). > > Where would you see that "making them const as needed" that you based your > reply on? :) I admit "making them const as needed" is badly phrased; I meant whether the container iterated-over is const to begin with, or can be made const in the range-for to avoid a detach/deep-copy ... etc, depending on the code and what it does. The same for the first argument of the range-for loop. The "addition"/change I was mainly talking about is the micro-optimization of not calling toString() twice; I think such basic coding practice changes are OK in this context. REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
