D23902: [KCoreDirLister] replace deprecated foreach with range-for

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:1044
> It's generally considered bad practice to return a const value, because the caller makes a copy anyway, so this doesn't guarantee anything. And it removes the benefits of rvalues that can be moved, since C++11.
> 
> I can see how range-for actually benefits from this, though.
> 
> It just seems that the generally agreed solution is to return non-const (for the other benefits) and use a local const variable as intermediary when using this in a range-for.

Noted.

> dfaure wrote in kcoredirlister.cpp:1960
> const

(Note to self, "we need a copy" doesn't necessarily mean a non-const copy).

> dfaure wrote in kcoredirlister.cpp:2073
> `holders.count()`, to match what the orig code was doing

Ouch, right.

> dfaure wrote in kcoredirlister.cpp:2222
> Why not just `qAsConst(listDirs)`? Did you identify a risk that the body of the for loop modifies lstDirs?

I thought that way it doesn't get cast (with qAsConst) twice, as lstDirs is used in another for loop a couple of lines down. But I was wrong, qAsConst doesn't copy its arguments, so qAsConst is better suited.

However this loop doesn't look like it modifies lstDirs.

> kcoredirlister.cpp:2237
>  
> -    Q_FOREACH (const QUrl &dir, lstDirs) {
> +    for (const QUrl &dir : dirs) {
>          KFileItemList deletedItems;

But this one might change lstDirs, as itemsDeleted is emitted, which calls deleteDir() and forgetDirs(); I am not sure though. I'll use a const var for this loop.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190914/cfc8ef32/attachment-0001.html>