Solid-device-automounter/kcm: Convert some foreach
ClosedPublic

Authored by meven on Jan 31 2020, 8:24 AM.

Details

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
meven created this revision.Jan 31 2020, 8:24 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 31 2020, 8:24 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Jan 31 2020, 8:24 AM
alex added a subscriber: alex.Jan 31 2020, 10:36 AM

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 !

In D27052#603798, @alex wrote:

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/

alex added a comment.Jan 31 2020, 11:10 AM

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 !

meven updated this revision to Diff 74803.Jan 31 2020, 3:25 PM

Add intermediate const variable for function

ervin accepted this revision.Feb 12 2020, 1:09 PM

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

This revision is now accepted and ready to land.Feb 12 2020, 1:09 PM
meven updated this revision to Diff 75840.Feb 17 2020, 4:24 PM
meven marked an inline comment as done.

Add a space

meven added inline comments.Feb 17 2020, 4:24 PM
solid-device-automounter/kded/DeviceAutomounter.cpp
73

Unfortunately this is not possible line 78 requires a non-const ref to volume.

This revision was automatically updated to reflect the committed changes.
ervin added inline comments.Feb 17 2020, 4:57 PM
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.