D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
This revision was automatically updated to reflect the committed changes. Closed by commit R241:80ccfabb06ff: Allow move semantics to be generated for KFileItem. The existing copy… (authored by markg). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10341?vs=26648=26755 REVISION DETAIL https://phabricator.kde.org/D10341 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: markg, dfaure, mwolff Cc: ngraham, #frameworks, michaelh
D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Ah yeah, of course. Can't be inlined, since the impl needs to see the private class. Forget what I said, out-of-line it is, and yes, do keep the unittest ;) It at least guards against a move ctor that would do nothing, for instance. REPOSITORY R241 KIO BRANCH kfileitem_move REVISION DETAIL https://phabricator.kde.org/D10341 To: markg, dfaure, mwolff Cc: ngraham, #frameworks, michaelh
D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
markg added a comment.
In https://phabricator.kde.org/D10341#202720, @markg wrote:
> In https://phabricator.kde.org/D10341#202704, @dfaure wrote:
>
> > I like the idea of enabling moves for KFileItem very much.
> >
> > But here's a fun fact: your unittest passes even without the rest of the
patch.
> >
> > PASS : KFileItemTest::testMove()
> >
> >
> > That's because std::move() doesn't move, it only makes the argument
eligible for e.g. a move ctor, but will call a copy ctor if there's no move
ctor. So that test is not really testing that move works ;)
> > That's always a bit tricky to test, I guess, because one can't really
rely on the state of the moved-from object to be anything in particular. And we
want =default, not to implement some counters in there.
> > Oh well, then maybe there's no real way to unittest that moving works.
>
>
> I know, i - somewhat - mentioned it ;)
> //"New test for move semantics (it passes, would probably pass without as
well but just be a copy)."//
>
> The test might be rather pointless as is, but running that test though
callgrind does show move semantics which is why i added it.
> Do i just remove it?
>
> > Anyhow, the thing I'm wondering is the following: does the rule-of-zero
lead to more opportunities for optimizations by the compiler, who can see "from
the outside" that the 5 special members are the default generated ones, while
your patch "hides" the implementation, lowering the visibility for the compiler
in the rest of the code? Well, one could just move the 5 "=default" to the
header to fix that.
>
> There was an issue with that... I don't quite remember what it was. Let me
try again... (to be continued)
- continued --
It doesn't compile...
I don't know why or how to fix it, you might :)
This is the message:
In file included from /usr/include/qt/QtCore/QSharedData:1:0,
from /home/mark/GitProjects/kio/src/core/udsentry.h:26,
from
/home/mark/GitProjects/kio_build/src/core/kio/udsentry.h:1,
from /home/mark/GitProjects/kio/src/core/kfileitem.h:26,
from
/home/mark/GitProjects/kio/src/core/kcoredirlister.h:24,
from
/home/mark/GitProjects/kio/src/core/kcoredirlister.cpp:23:
/usr/include/qt/QtCore/qshareddata.h: In instantiation of
‘QSharedDataPointer::QSharedDataPointer(const QSharedDataPointer&) [with
T = KFileItemPrivate]’:
/home/mark/GitProjects/kio/src/core/kfileitem.h:121:5: required from here
/usr/include/qt/QtCore/qshareddata.h:92:84: error: invalid use of
incomplete type ‘class KFileItemPrivate’
inline QSharedDataPointer(const QSharedDataPointer ) : d(o.d) {
if (d) d->ref.ref(); }
~~~^~~
In file included from
/home/mark/GitProjects/kio/src/core/kcoredirlister.h:24:0,
from
/home/mark/GitProjects/kio/src/core/kcoredirlister.cpp:23:
/home/mark/GitProjects/kio/src/core/kfileitem.h:35:7: note: forward
declaration of ‘class KFileItemPrivate’
class KFileItemPrivate;
^~~~
kfileitem.h:121 is: KFileItem(const KFileItem&) = default;
It spawns that error about 14 times on various places...
All errors: https://p.sc2.nl/BJviiMKUG
And the code as diff on top of this change (should apply cleanly):
https://p.sc2.nl/rJwAifKUM
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10341
To: markg, dfaure, mwolff
Cc: ngraham, #frameworks, michaelh
D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
markg added a comment. In https://phabricator.kde.org/D10341#202704, @dfaure wrote: > I like the idea of enabling moves for KFileItem very much. > > But here's a fun fact: your unittest passes even without the rest of the patch. > > PASS : KFileItemTest::testMove() > > > That's because std::move() doesn't move, it only makes the argument eligible for e.g. a move ctor, but will call a copy ctor if there's no move ctor. So that test is not really testing that move works ;) > That's always a bit tricky to test, I guess, because one can't really rely on the state of the moved-from object to be anything in particular. And we want =default, not to implement some counters in there. > Oh well, then maybe there's no real way to unittest that moving works. I know, i - somewhat - mentioned it ;) //"New test for move semantics (it passes, would probably pass without as well but just be a copy)."// The test might be rather pointless as is, but running that test though callgrind does show move semantics which is why i added it. Do i just remove it? > Anyhow, the thing I'm wondering is the following: does the rule-of-zero lead to more opportunities for optimizations by the compiler, who can see "from the outside" that the 5 special members are the default generated ones, while your patch "hides" the implementation, lowering the visibility for the compiler in the rest of the code? Well, one could just move the 5 "=default" to the header to fix that. There was an issue with that... I don't quite remember what it was. Let me try again... (to be continued) My preference is = default in the header. > Of course in both cases (rule-of-zero or 5*=default in the header), it means we are tied to the 5 defaults for all of KF5, for BIC reasons. But that seems rather sensible in this case (the only member will always be the d pointer, and it's unlikely we'll move away from refcounting...). > And in fact the other reason against rule-of-zero is that we can't just remove the copy-ctor and operator=, that would be BIC (existing code links to it). Ha, i tried and found out :) (before posting it here). That's why i went for the = default version (rule-of-five-defaults) to prevent that BIC. > In summary: this looks good to me, I'm just wondering if inlining the 5 "=default" wouldn't be better, for optimization purposes. You know the real funny thing here? KFileItemList... It's a crappy QList with no move semantics.. I want to deprecate it (working on a patch for that now) by replacing it with a "using KFileItemListV2 = std::vector" The biggest improvements can be made by having move semantics in KFileItemListV2. It would also deprecate a load of KIO functions that all expose KFileItemList (either as argument or as return value). The downside? Well, you probably guessed it already. A lot of deprecated warnings and current code that needs to stay working for the KF5 lifetime thus in the short term this might actually be a performance loss :( I'll post a patch for this soon for you to judge :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10341 To: markg, dfaure, mwolff Cc: ngraham, #frameworks, michaelh
D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
dfaure added a comment. I like the idea of enabling moves for KFileItem very much. But here's a fun fact: your unittest passes even without the rest of the patch. PASS : KFileItemTest::testMove() That's because std::move() doesn't move, it only makes the argument eligible for e.g. a move ctor, but will call a copy ctor if there's no move ctor. So that test is not really testing that move works ;) That's always a bit tricky to test, I guess, because one can't really rely on the state of the moved-from object to be anything in particular. And we want =default, not to implement some counters in there. Oh well, then maybe there's no real way to unittest that moving works. Anyhow, the thing I'm wondering is the following: does the rule-of-zero lead to more opportunities for optimizations by the compiler, who can see "from the outside" that the 5 special members are the default generated ones, while your patch "hides" the implementation, lowering the visibility for the compiler in the rest of the code? Well, one could just move the 5 "=default" to the header to fix that. Of course in both cases (rule-of-zero or 5*=default in the header), it means we are tied to the 5 defaults for all of KF5, for BIC reasons. But that seems rather sensible in this case (the only member will always be the d pointer, and it's unlikely we'll move away from refcounting...). And in fact the other reason against rule-of-zero is that we can't just remove the copy-ctor and operator=, that would be BIC (existing code links to it). In summary: this looks good to me, I'm just wondering if inlining the 5 "=default" wouldn't be better, for optimization purposes. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10341 To: markg, dfaure, mwolff Cc: ngraham, #frameworks, michaelh
D10341: Allow move semantics to be generated for KFileItem. The existing copy constructor, destructor and copy assignment operator are now also generated by the compiler.
markg created this revision. markg added reviewers: dfaure, mwolff. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. markg requested review of this revision. REVISION SUMMARY This allows the compiler to generate: - Move constructor - Move assingment - Copy constructor - Copy assignment - Destructor This in turn allows further KFileItem optimization throughout KIO and Dolphin. Also added a quite minimal test to see if move semantics work. As implemented now it roughly follows the "rule-of-five-default": http://scottmeyers.blogspot.nl/2014/03/a-concern-about-rule-of-zero.html I was tempted to go for the "rule-of-zero" which means not implementing any of those functions (thus the compiler generates them), but that - in my opinion - is not really clear as it's easy to add the destructor and then be surprised by not having move semantics anymore. TEST PLAN New test for move semantics (it passes, would probably pass without as well but just be a copy). Existing relevant tests (kfileitemtest and kdirmodeltest) all pass just fine. Running the new "testMove" through callgrind shows that the move constructor and assignment operator are really being used by that test. REPOSITORY R241 KIO BRANCH kfileitem_move REVISION DETAIL https://phabricator.kde.org/D10341 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: markg, dfaure, mwolff Cc: #frameworks, michaelh, ngraham
