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
