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

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> meven wrote in DeviceAutomounter.cpp:73
> Unfortunately this is not possible line 78 requires a non-const ref to volume.

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:efdf88ec48b4: Solid-device-automounter/kcm: Convert some 
foreach (authored by meven).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D27052?vs=75840=75841#toc

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27052?vs=75840=75841

REVISION DETAIL
  https://phabricator.kde.org/D27052

AFFECTED FILES
  solid-device-automounter/kcm/DeviceAutomounterKCM.cpp
  solid-device-automounter/kcm/DeviceModel.cpp
  solid-device-automounter/kded/DeviceAutomounter.cpp

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> ervin wrote in 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).

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D27052_1

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven updated this revision to Diff 75840.
meven marked an inline comment as done.
meven added a comment.


  Add a space

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27052?vs=74803=75840

BRANCH
  arcpatch-D27052_1

REVISION DETAIL
  https://phabricator.kde.org/D27052

AFFECTED FILES
  solid-device-automounter/kcm/DeviceAutomounterKCM.cpp
  solid-device-automounter/kcm/DeviceModel.cpp
  solid-device-automounter/kded/DeviceAutomounter.cpp

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kevin Ottens]

ervin accepted this revision.
ervin added a comment.
This revision is now accepted and ready to land.


  Just a nitpick, your call if you want to tackle it or not.

INLINE COMMENTS

> DeviceAutomounter.cpp:73
> +const QList volumes = 
> Solid::Device::listFromType(Solid::DeviceInterface::StorageVolume);
> +for(Solid::Device volume : volumes) {
>  // sa can be 0 (e.g. for the swap partition)

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  arcpatch-D27052

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven updated this revision to Diff 74803.
meven added a comment.


  Add intermediate const variable for function

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27052?vs=74742=74803

BRANCH
  arcpatch-D27052

REVISION DETAIL
  https://phabricator.kde.org/D27052

AFFECTED FILES
  solid-device-automounter/kcm/DeviceAutomounterKCM.cpp
  solid-device-automounter/kcm/DeviceModel.cpp
  solid-device-automounter/kded/DeviceAutomounter.cpp

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Alexander Lohnau]

alex added a comment.


  > 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 !

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven added a comment.


  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/

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Alexander Lohnau]

alex added a comment.


  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 !

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27052

To: meven, broulik, ervin, #plasma
Cc: alex, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Méven Car]

meven created this revision.
meven added reviewers: broulik, ervin, Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
meven requested review of this revision.

TEST PLAN
  builds

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27052

AFFECTED FILES
  solid-device-automounter/kcm/DeviceAutomounterKCM.cpp
  solid-device-automounter/kcm/DeviceModel.cpp
  solid-device-automounter/kded/DeviceAutomounter.cpp

To: meven, broulik, ervin, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart