<table><tr><td style="">markg added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D10341" rel="noreferrer">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D10341#202720" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10341#202720</a>, <a href="https://phabricator.kde.org/p/markg/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@markg</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D10341#202704" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10341#202704</a>, <a href="https://phabricator.kde.org/p/dfaure/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@dfaure</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I like the idea of enabling moves for KFileItem very much.</p>
<p>But here's a fun fact: your unittest passes even without the rest of the patch.</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">PASS : KFileItemTest::testMove()</pre></div>
<p>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 ;)<br />
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.<br />
Oh well, then maybe there's no real way to unittest that moving works.</p></div>
</blockquote>
<p>I know, i - somewhat - mentioned it ;)<br />
<em>"New test for move semantics (it passes, would probably pass without as well but just be a copy)."</em></p>
<p>The test might be rather pointless as is, but running that test though callgrind does show move semantics which is why i added it.<br />
Do i just remove it?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>There was an issue with that... I don't quite remember what it was. Let me try again... (to be continued)</p></div>
</blockquote>
<ul class="remarkup-list">
<li class="remarkup-list-item">continued --</li>
</ul>
<p>It doesn't compile...<br />
I don't know why or how to fix it, you might :)<br />
This is the message:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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;
^~~~~~~~~~~~~~~~</pre></div>
<p>kfileitem.h:121 is: KFileItem(const KFileItem&) = default; <br />
It spawns that error about 14 times on various places...<br />
All errors: <a href="https://p.sc2.nl/BJviiMKUG" class="remarkup-link" target="_blank" rel="noreferrer">https://p.sc2.nl/BJviiMKUG</a><br />
And the code as diff on top of this change (should apply cleanly): <a href="https://p.sc2.nl/rJwAifKUM" class="remarkup-link" target="_blank" rel="noreferrer">https://p.sc2.nl/rJwAifKUM</a></p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10341" rel="noreferrer">https://phabricator.kde.org/D10341</a></div></div><br /><div><strong>To: </strong>markg, dfaure, mwolff<br /><strong>Cc: </strong>ngraham, Frameworks, michaelh<br /></div>