D23813: Port away from foreach loops over arguments without calls to owner class
ahmadsamir added inline comments. INLINE COMMENTS > kossebau wrote in kkeysequencewidget.cpp:127 > Still unsure what you mean with the constness, as the QHash `shortcuts` has > been const all the time, and thus the access methods and its returned > references. If you meant the initially proposed `const QKeySequence &seq = > it.key();` that was just an alias reference to something const ref before > (`it.key()`), whose idea was to not touch the other existing code as well as > make it more obvious to the code reader what `it.key()` is. It did not change > any constness. > > Back to your original comment: > So here my 2 penny collected over decades: avoid that. One change/aspect at a > time. No additional clean-up., as basic as it is (even no whitespace changes, > unless line touched anyway). > > - The commit message might miss to mention that change, or make it more > complicated to read because it lists all the while-at-it changes. > - There are no obvious changes, unless documented. What is clear to the > commit creator, might be unclear to the commit reader, as they have another > context > - Line-wise commit annotation mark-up will be set for lines which are changed > while not relevant for the actual main change (which only would be mentioned > in the commit message first line/title) > - One is concentrated on the main change, and might miss important details > relevant to that other change, and introduce regressions. > > You may discard these 2 penny of mine, but let's talk again in some years ;) > Better though ask the search engine for what other people recommend as best > commit practice & compare. Still you are free to collect your own experience: > if young people only did what old people tell them, new discoveries would > never be made ;) But most of the times... I do try to keep commits atomic, and detail my changes in the commit message. (And no, I won't discard your 2 pennies, I am too poor, experience-wise, to afford that). What I should have done was read the code starting at the top, where shortcuts is declared const. Anyway thanks for explaining things, that's always appreciated. :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
kossebau added inline comments. INLINE COMMENTS > ahmadsamir wrote in kkeysequencewidget.cpp:127 > 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. Still unsure what you mean with the constness, as the QHash `shortcuts` has been const all the time, and thus the access methods and its returned references. If you meant the initially proposed `const QKeySequence &seq = it.key();` that was just an alias reference to something const ref before (`it.key()`), whose idea was to not touch the other existing code as well as make it more obvious to the code reader what `it.key()` is. It did not change any constness. Back to your original comment: So here my 2 penny collected over decades: avoid that. One change/aspect at a time. No additional clean-up., as basic as it is (even no whitespace changes, unless line touched anyway). - The commit message might miss to mention that change, or make it more complicated to read because it lists all the while-at-it changes. - There are no obvious changes, unless documented. What is clear to the commit creator, might be unclear to the commit reader, as they have another context - Line-wise commit annotation mark-up will be set for lines which are changed while not relevant for the actual main change (which only would be mentioned in the commit message first line/title) - One is concentrated on the main change, and might miss important details relevant to that other change, and introduce regressions. You may discard these 2 penny of mine, but let's talk again in some years ;) Better though ask the search engine for what other people recommend as best commit practice & compare. Still you are free to collect your own experience: if young people only did what old people tell them, new discoveries would never be made ;) But most of the times... REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
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
D23813: Port away from foreach loops over arguments without calls to owner class
kossebau added inline comments. INLINE COMMENTS > ahmadsamir wrote in kkeysequencewidget.cpp:127 > I understand. but I was talking about this case specifically, it's about > foreach two arguments, and making them const as needed, so that seqAsString > change is in inline with replacing foreach with range-for. 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? :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
ahmadsamir added inline comments. INLINE COMMENTS > kossebau wrote in kkeysequencewidget.cpp:127 > As someone who looked at a lot of commit history, my recommendation is: don't > do in the same commit. Make it a separate commit with a dedicated commit > message. > While-at-it changes are annoying for future code history readers, which > includes one older-self. So not only for that reason be friendly to them :) I understand. but I was talking about this case specifically, it's about foreach two arguments, and making them const as needed, so that seqAsString change is in inline with replacing foreach with range-for. REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
kossebau added inline comments. INLINE COMMENTS > ahmadsamir wrote in kkeysequencewidget.cpp:127 > My two (inexperienced) pennyworth: if it makes sense, and should have been > done to begin with (so most likely it's an oversight), I'd always go for it > (who knows how long it'll be before that bit of code is looked at again). As someone who looked at a lot of commit history, my recommendation is: don't do in the same commit. Make it a separate commit with a dedicated commit message. While-at-it changes are annoying for future code history readers, which includes one older-self. So not only for that reason be friendly to them :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
ahmadsamir added inline comments. INLINE COMMENTS > kossebau wrote in kkeysequencewidget.cpp:127 > I try (hard, there are many temptations when looking at all exisiting loop > code) to usually do not too much other improvements but stay on-topic of > commit message change, but will do an exception given you asked for it, and > the change will not confuse commit history reader too much :) My two (inexperienced) pennyworth: if it makes sense, and should have been done to begin with (so most likely it's an oversight), I'd always go for it (who knows how long it'll be before that bit of code is looked at again). REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
kossebau added a comment. Thanks for review :) INLINE COMMENTS > dfaure wrote in kkeysequencewidget.cpp:127 > This could even be `const QString seq = it.key().toString();` so that > toString() is only called once. I try (hard, there are many temptations when looking at all exisiting loop code) to usually do not too much other improvements but stay on-topic of commit message change, but will do an exception given you asked for it, and the change will not confuse commit history reader too much :) REPOSITORY R263 KXmlGui REVISION DETAIL https://phabricator.kde.org/D23813 To: kossebau, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
This revision was automatically updated to reflect the committed changes. Closed by commit R263:02ee352df1af: Port away from foreach loops over arguments without calls to owner class (authored by kossebau). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D23813?vs=65716&id=65808#toc REPOSITORY R263 KXmlGui CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23813?vs=65716&id=65808 REVISION DETAIL https://phabricator.kde.org/D23813 AFFECTED FILES autotests/kxmlgui_unittest.cpp autotests/testguiclient.h autotests/testxmlguiwindow.h src/kkeysequencewidget.cpp src/kshortcutschemeshelper.cpp src/kxmlguiclient.cpp src/kxmlguifactory.cpp src/kxmlguiversionhandler.cpp To: kossebau, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.
(Thread usage is completely unlikely in users of this code, on the containers
being passed in, this is really 100% GUI code; it's up to the caller to
synchronize this correctly anyway, in that unlikely event...)
INLINE COMMENTS
> kkeysequencewidget.cpp:127
> +for (auto it = shortcuts.begin(), end = shortcuts.end(); it != end;
> ++it) {
> +const QKeySequence &seq = it.key();
> +for (const KGlobalShortcutInfo &info : it.value()) {
This could even be `const QString seq = it.key().toString();` so that
toString() is only called once.
REPOSITORY
R263 KXmlGui
BRANCH
portmoreforeachmethodargswithotrecursivecalls
REVISION DETAIL
https://phabricator.kde.org/D23813
To: kossebau, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23813: Port away from foreach loops over arguments without calls to owner class
kossebau created this revision. kossebau added a reviewer: dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kossebau requested review of this revision. REVISION SUMMARY There is some small risk here: - overseen call chains which still call the owner and modify the container - other threads might access the same containers, even if class is not designed to be thread-safe, but wrong usages are currently caught mostly by the container copy GIT_SILENT REPOSITORY R263 KXmlGui BRANCH portmoreforeachmethodargswithotrecursivecalls REVISION DETAIL https://phabricator.kde.org/D23813 AFFECTED FILES autotests/kxmlgui_unittest.cpp autotests/testguiclient.h autotests/testxmlguiwindow.h src/kkeysequencewidget.cpp src/kshortcutschemeshelper.cpp src/kxmlguiclient.cpp src/kxmlguifactory.cpp src/kxmlguiversionhandler.cpp To: kossebau, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
