builds
Details
- Reviewers
broulik ervin - Group Reviewers
Plasma - Commits
- R119:efdf88ec48b4: Solid-device-automounter/kcm: Convert some foreach
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- arcpatch-D27052_1
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 22583 Build 22601: arc lint + arc unit
Hello,
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.
solid-device-automounter/kded/DeviceAutomounter.cpp | ||
---|---|---|
73 | 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). |
solid-device-automounter/kded/DeviceAutomounter.cpp | ||
---|---|---|
73 | Unfortunately this is not possible line 78 requires a non-const ref to volume. |
solid-device-automounter/kded/DeviceAutomounter.cpp | ||
---|---|---|
73 | 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. |