- Group Reviewers
- R119:efdf88ec48b4: Solid-device-automounter/kcm: Convert some foreach
I have a question regarding your changed.
you sometimes declare the variables you want to loop over before the loop, but sometimes in the loop.
For instance in DeviceModel.cpp line 135 it is declared as a const before the loop and in line 139 it is directly used in the loop.
In one of my previous revisions I have implemented it like in line 139 and https://phabricator.kde.org/D26912?id=74352#inline-152211 and changes were requested.
Am I missing conceptual differences between these use cases ?
Thanks again for your expertise !
The important thing is as long as you don't need to change in a loop the container, your container should be const to be used in a for loop, so that semantically the for loop is equivalent to the foreach version, but avoiding the copying foreach did.
So either your container is a variable, it needs to be const, if not use qAsConst, and if it is a function you don't need to do anything if the function is const (keys()), if not the best practice is to introduce an intermediate const variable.
The long version about this Q_FOREACH / foreach is at https://www.kdab.com/goodbye-q_foreach/
if it is a function you don't need to do anything if the function is const (keys()), if not the best practice is to introduce an intermediate const variable
That makes everything clear, thank you very much !
Just a nitpick, your call if you want to tackle it or not.
nitpick, feel free to ignore: would make sense to switch to a const ref here since we're touching the line anyway. in the same vein, "const auto" on the line before wouldn't be a bad idea either (if we ever switch to QVector for instance it'd be one less place where compilation would break).
const auto for volumes was possible though.
I wonder if it wouldn't have made sense to have a non const ref in the loop then. Would have avoided the (potential) copy and made clear this loop was modifying. Oh well.