[KCoreDirLister] replace deprecated foreach with range-for
ClosedPublic

Authored by ahmadsamir on Sep 12 2019, 1:18 PM.

Details

Summary

Also change directoriesForCanonicalPath() to return const

Test Plan

The code compiles and all tests pass (except kacltest, but that's
probably something on my machine)

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Sep 12 2019, 1:18 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 12 2019, 1:18 PM
ahmadsamir requested review of this revision.Sep 12 2019, 1:18 PM
dfaure requested changes to this revision.Sep 14 2019, 12:45 PM
dfaure added inline comments.
src/core/kcoredirlister.cpp
285

The method is const, so this qAsConst isn't needed.

1038

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.

1473

I guess this needs a local const var to hold listers+holders, so that the container that we're iterating on, is const?

1491

same here

1958–1959

const

1966–1967

const

2072

holders.count(), to match what the orig code was doing

2221

Why not just qAsConst(listDirs)? Did you identify a risk that the body of the for loop modifies lstDirs?

This revision now requires changes to proceed.Sep 14 2019, 12:45 PM
ahmadsamir updated this revision to Diff 66073.Sep 14 2019, 5:37 PM
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.

Fix stuff mentioned in code review

ahmadsamir added inline comments.Sep 14 2019, 5:38 PM
src/core/kcoredirlister.cpp
1038

Noted.

1958–1959

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

2072

Ouch, right.

2221

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.

2235–2236

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.

dfaure requested changes to this revision.Sep 17 2019, 7:34 PM
dfaure added inline comments.
src/core/kcoredirlister.cpp
826–827

Well, then you need a local const variable here.

1073–1074

and here

1079–1080

and here

1154–1155

and here

2221

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

2235–2236

well spotted.

2246

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

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

This revision now requires changes to proceed.Sep 17 2019, 7:34 PM
ahmadsamir marked 10 inline comments as done.Sep 17 2019, 10:48 PM

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.

src/core/kcoredirlister.cpp
826–827

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

2221

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

2246

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.

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.

ahmadsamir updated this revision to Diff 66393.Sep 18 2019, 3:49 PM
ahmadsamir marked 2 inline comments as done.

Fix patch

dfaure accepted this revision.Sep 18 2019, 7:06 PM
This revision is now accepted and ready to land.Sep 18 2019, 7:06 PM
This revision was automatically updated to reflect the committed changes.

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

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.

dfaure added inline comments.Sep 21 2019, 12:17 PM
src/core/kcoredirlister.cpp
1038

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

ahmadsamir added inline comments.Sep 22 2019, 3:09 PM
src/core/kcoredirlister.cpp
1038

Noted. :)
(Thanks for sharing).

dfaure added a comment.Oct 4 2019, 2:23 PM

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<int> mylist = foo(); copies instead of moving.
So yeah, better not do that.

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

dfaure added a comment.Oct 4 2019, 3:53 PM

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

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