Port away from foreach loops over arguments without calls to owner class
ClosedPublic

Authored by kossebau on Tue, Sep 10, 12:55 AM.

Details

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

Diff Detail

Repository
R263 KXmlGui
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Tue, Sep 10, 12:55 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptTue, Sep 10, 12:55 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Tue, Sep 10, 12:55 AM
dfaure accepted this revision.Tue, Sep 10, 7:51 PM

(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...)

src/kkeysequencewidget.cpp
127

This could even be const QString seq = it.key().toString(); so that toString() is only called once.

This revision is now accepted and ready to land.Tue, Sep 10, 7:51 PM
This revision was automatically updated to reflect the committed changes.

Thanks for review :)

src/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 :)

ahmadsamir added inline comments.
src/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).

kossebau added inline comments.Thu, Sep 12, 11:18 AM
src/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 :)

ahmadsamir added inline comments.Thu, Sep 12, 1:29 PM
src/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.

kossebau added inline comments.Thu, Sep 12, 1:50 PM
src/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? :)

ahmadsamir added inline comments.Thu, Sep 12, 4:33 PM
src/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.

kossebau added inline comments.Thu, Sep 12, 5:21 PM
src/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...

ahmadsamir added inline comments.Thu, Sep 12, 6:08 PM
src/kkeysequencewidget.cpp
127

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. :)