Also change directoriesForCanonicalPath() to return const
Details
The code compiles and all tests pass (except kacltest, but that's
probably something on my machine)
Diff Detail
- Repository
- R241 KIO
- Branch
- ahmad/foreach (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 16428 Build 16446: arc lint + arc unit
src/core/kcoredirlister.cpp | ||
---|---|---|
285 | The method is const, so this qAsConst isn't needed. | |
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. | |
1474 | I guess this needs a local const var to hold listers+holders, so that the container that we're iterating on, is const? | |
1492 | same here | |
1960 | const | |
1968 | const | |
2073 | holders.count(), to match what the orig code was doing | |
2222 | Why not just qAsConst(listDirs)? Did you identify a risk that the body of the for loop modifies lstDirs? |
src/core/kcoredirlister.cpp | ||
---|---|---|
1044 | Noted. | |
1960 | (Note to self, "we need a copy" doesn't necessarily mean a non-const copy). | |
2073 | Ouch, right. | |
2222 | 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. | |
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. |
src/core/kcoredirlister.cpp | ||
---|---|---|
833 | Well, then you need a local const variable here. | |
1079 | and here | |
1084 | and here | |
1158 | and here | |
2222 | Did you forget to change it to qAsConst(listDirs) ? | |
2237 | 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] |
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 | ||
---|---|---|
833 | Right. canonicalUrls can change while iterating (me stupid). | |
2222 | 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)
- 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!)
- 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.
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.
src/core/kcoredirlister.cpp | ||
---|---|---|
1044 | Actually, I was wrong. 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). |
src/core/kcoredirlister.cpp | ||
---|---|---|
1044 | Noted. :) |
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.
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... :)).
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.
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.