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

[Ahmad Samir]

ahmadsamir added a comment.


  In D23902#542046 , @dfaure wrote:
  
  > In D23902#542037 , @ahmadsamir 
wrote:
  >
  > > In D23902#542026 , @dfaure 
wrote:
  > >
  > > > For the record, JFBastien was actually wrong. Calling .begin() on a 
const return value does call the const overload. Testcase 
http://www.davidfaure.fr/kde/const_retval.cpp
  > >
  > >
  > > That looks like a failure in communication, either one of you _assumed_ 
something but didn't tell the other. :D
  >
  >
  > You and I are both entitled to our own opinion, I know what I asked and 
what the reply was :-P
  >  (I tried to reach him afterwards via linkedin and cppcon slack, no success)
  >
  > >> But returning const QList would inhibit move semantics, e.g. `QList 
mylist = foo();` copies instead of moving.
  > >>  So yeah, better not do that.
  > > 
  > > And it would detach (I don't know why it feels like detaching for Qt 
containers is like a sword hanging over all, especially new, developers' heads; 
so implicit sharing is great, except you have to worry about the container 
detaching for the rest of its natural life... :)).
  >
  > No, a copy doesn't detach. A non-const method call on a copy detaches.
  
  
  See? I actually was talking about calling a non-const method, begin(), on a 
non-const Qt container (or a copy of it), that would make it detach even if it 
never gets actually modified in the loop, that's what I was thinking in my 
head. So I _assumed_ it got the point across but because I posted that right 
under your post about move semantics that kicked the point out the window; so 
you assumed I was talking about move semantics. Assumptions are bad, man I am 
telling you :)
  
  > And usually you don't have to worry about that. Detaching happens when it 
should, i.e. when modifying a copy, that's intended. The problem is range-for, 
which came up later, and which throws ease-of-use a little bit out of the 
window indeed.
  
  Well, I _guess_ the devs who wrote the range for code did that with STL 
containers in mind, where you're OK if you don't change the container _size_ 
(IIUC). They probably weren't thinking about the implicitly shared containers 
created by the Qt devs?
  
  Anyway, thanks for sharing.

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

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

[David Faure]

dfaure added a comment.


  In D23902#542037 , @ahmadsamir 
wrote:
  
  > In D23902#542026 , @dfaure wrote:
  >
  > > For the record, JFBastien was actually wrong. Calling .begin() on a const 
return value does call the const overload. Testcase 
http://www.davidfaure.fr/kde/const_retval.cpp
  >
  >
  > That looks like a failure in communication, either one of you _assumed_ 
something but didn't tell the other. :D
  
  
  You and I are both entitled to our own opinion, I know what I asked and what 
the reply was :-P
  (I tried to reach him afterwards via linkedin and cppcon slack, no success)
  
  >> But returning const QList would inhibit move semantics, e.g. `QList 
mylist = foo();` copies instead of moving.
  >>  So yeah, better not do that.
  > 
  > And it would detach (I don't know why it feels like detaching for Qt 
containers is like a sword hanging over all, especially new, developers' heads; 
so implicit sharing is great, except you have to worry about the container 
detaching for the rest of its natural life... :)).
  
  No, a copy doesn't detach. A non-const method call on a copy detaches.
  
  And usually you don't have to worry about that. Detaching happens when it 
should, i.e. when modifying a copy, that's intended. The problem is range-for, 
which came up later, and which throws ease-of-use a little bit out of the 
window indeed.

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

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

[Ahmad Samir]

ahmadsamir added a comment.


  In D23902#542026 , @dfaure wrote:
  
  > For the record, JFBastien was actually wrong. Calling .begin() on a const 
return value does call the const overload. Testcase 
http://www.davidfaure.fr/kde/const_retval.cpp
  
  
  That looks like a failure in communication, either one of you _assumed_ 
something but didn't tell the other. :D
  
  > But returning const QList would inhibit move semantics, e.g. `QList 
mylist = foo();` copies instead of moving.
  >  So yeah, better not do that.
  
  And it would detach (I don't know why it feels like detaching for Qt 
containers is like a sword hanging over all, especially new, developers' heads; 
so implicit sharing is great, except you have to worry about the container 
detaching for the rest of its natural life... :)).

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

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

[David Faure]

dfaure added a comment.


  For the record, JFBastien was actually wrong. Calling .begin() on a const 
return value does call the const overload. Testcase 
http://www.davidfaure.fr/kde/const_retval.cpp
  
  But returning const QList would inhibit move semantics, e.g. `QList 
mylist = foo();` copies instead of moving.
  So yeah, better not do that.

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

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

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:1044
> Actually, I was wrong.
> I talked to the head of the LLVM/clang development team at Apple and he told 
> me that returning a const value has zero effect whatsoever. I.e. in the 
> version of your patch where you were returning a const value, the range for 
> would *still* call the non-const begin() and end(). Good thing I made you 
> change that :-)
> 
> His plan is to make a future version of the C++ standard deprecate const 
> arguments and const return values in function declarations. (toplevel const 
> only, const-ref is fine of course).

Noted. :)
(Thanks for sharing).

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

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

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kcoredirlister.cpp:1044
> Noted.

Actually, I was wrong.
I talked to the head of the LLVM/clang development team at Apple and he told me 
that returning a const value has zero effect whatsoever. I.e. in the version of 
your patch where you were returning a const value, the range for would *still* 
call the non-const begin() and end(). Good thing I made you change that :-)

His plan is to make a future version of the C++ standard deprecate const 
arguments and const return values in function declarations. (toplevel const 
only, const-ref is fine of course).

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

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

[David Faure]

dfaure added a comment.


  Too late, what's pushed is pushed (no force-push allowed, it would break 
existing checkouts with non-pushed work on top).
  
  No big deal.

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

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

[Ahmad Samir]

ahmadsamir added a comment.


  I am sorry, I forgot to use --verbatim, please remove the line about 
directoriesForCanonicalPath() returning const from the commit message :/

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

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

[David Faure]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:3c955240e87b: [KCoreDirLister] replace deprecated foreach 
with range-for (authored by ahmadsamir, committed by dfaure).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23902?vs=66393&id=66407

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

AFFECTED FILES
  src/core/kcoredirlister.cpp

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  ahmad/foreach (branched from master)

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

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Ahmad Samir]

ahmadsamir updated this revision to Diff 66393.
ahmadsamir marked 2 inline comments as done.
ahmadsamir added a comment.


  Fix patch

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23902?vs=66073&id=66393

BRANCH
  ahmad/foreach (branched from master)

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

AFFECTED FILES
  src/core/kcoredirlister.cpp

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  Don't confuse the two rules of porting to range-for (for Qt containers)
  
  1. The container should be const, to avoid paying for a detach. So either a 
local const var, or qAsConst() over a non-const var (but never a temporary 
returned by a function!)
  2. The container should not be modified inside the loop. If it is, we'll get 
a crash. Much worse than a detach.
  
  You need the qAsConst if the container isn't const, EVEN if it's not modified 
in the 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

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

[Ahmad Samir]

ahmadsamir marked 10 inline comments as done.
ahmadsamir added a comment.


  I think I got all the bits I missed before (sorry about the mess).
  
  But I'll sleep on it anyway, will submit in the morning (usually I find 
mistakes in my code when I look at it again in the morning).
  
  Thanks.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:833
> Well, then you need a local const variable here.

Right. canonicalUrls can change while iterating (me stupid).

> dfaure wrote in kcoredirlister.cpp:2248
> Why did you remove the const in front of "auto kend"?
> 
> [alternatively, the next line could be ported to a range for, I guess]

I'll revert this bit altogether, I shouldn't have changed it at all.

And I will submit a separate patch to use a range for, for it.

> dfaure wrote in kcoredirlister.cpp:
> Did you forget to change it to `qAsConst(listDirs)` ?

It looks like this loop doesn't change lstDirs; or do you mean I should use 
qAsConst just in case?

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

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

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcoredirlister.cpp:833
>  qCDebug(KIO_CORE_DIRLISTER) << urlDir; // output urls, not qstrings, 
> since they might contain a password
> -Q_FOREACH (const QUrl &u, directoriesForCanonicalPath(urlDir)) {
> +for (const QUrl &u : directoriesForCanonicalPath(urlDir)) {
>  updateDirectory(u);

Well, then you need a local const variable here.

> kcoredirlister.cpp:1079
>  if (isDir) {
> -Q_FOREACH (const QUrl &dir, directoriesForCanonicalPath(url)) {
> +for (const QUrl &dir : directoriesForCanonicalPath(url)) {
>  handleFileDirty(dir); // e.g. for permission changes

and here

> kcoredirlister.cpp:1084
>  } else {
> -Q_FOREACH (const QUrl &dir, 
> directoriesForCanonicalPath(url.adjusted(QUrl::RemoveFilename | 
> QUrl::StripTrailingSlash))) {
> +for (const QUrl &dir : 
> directoriesForCanonicalPath(url.adjusted(QUrl::RemoveFilename | 
> QUrl::StripTrailingSlash))) {
>  QUrl aliasUrl(dir);

and here

> kcoredirlister.cpp:1158
>  QStringList fileUrls;
> -Q_FOREACH (const QUrl &url, 
> directoriesForCanonicalPath(dirUrl.adjusted(QUrl::RemoveFilename | 
> QUrl::StripTrailingSlash))) {
> +for (const QUrl &url : 
> directoriesForCanonicalPath(dirUrl.adjusted(QUrl::RemoveFilename | 
> QUrl::StripTrailingSlash))) {
>  QUrl urlInfo(url);

and here

> kcoredirlister.cpp:2248
> +auto kit = itemList->cbegin();
> +auto kend = itemList->cend();
>  for (; kit != kend; ++kit) {

Why did you remove the const in front of "auto kend"?

[alternatively, the next line could be ported to a range for, I guess]

> ahmadsamir wrote in kcoredirlister.cpp:
> 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.

Did you forget to change it to `qAsConst(listDirs)` ?

> ahmadsamir wrote in kcoredirlister.cpp:2237
> 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.

well spotted.

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

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

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

[Ahmad Samir]

ahmadsamir updated this revision to Diff 66073.
ahmadsamir marked 7 inline comments as done.
ahmadsamir retitled this revision from "[KCoreDirLister] replace foreach with 
range-for" to "[KCoreDirLister] replace deprecated foreach with range-for".
ahmadsamir added a comment.


  Fix stuff mentioned in code review

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23902?vs=65910&id=66073

BRANCH
  ahmad/foreach (branched from master)

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

AFFECTED FILES
  src/core/kcoredirlister.cpp

To: ahmadsamir, kde-frameworks-devel, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns