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<T>::QSharedDataPointer(const QSharedDataPointer<T>&) [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<T> &o) : 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
