Add move semantics support to KIO::UDSEntry.

Authored by markg on Feb 9 2018, 6:29 PM.



Adds move semantics support and a testcase (only useful in callgrind).
Right now there are 0 uses of these semantics.

Test Plan

Relevant tests (udsentrytest and kfileitemtest) pass.
Dolphin compiles and runs just fine with it.

Diff Detail

R241 KIO
No Linters Available
No Unit Test Coverage
markg created this revision.Feb 9 2018, 6:29 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 9 2018, 6:29 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
markg requested review of this revision.Feb 9 2018, 6:29 PM
markg updated this revision to Diff 26838.Feb 9 2018, 6:32 PM

Fix indentation.

markg added a comment.Feb 9 2018, 6:36 PM

Just a note.
The fact that currently no code is structured in a way that move semantics are used is... slightly annoying ;)
No as in not when running dolphin with it and profiling it to see UDSEntry uses. There "might" be code that uses move semantics when this patch is applied, but i haven't seen it yet.

So, more patches to come to fix that. I've already spotted one potential case in KCoreDirListerCache although that probably depends on more.

apol added a subscriber: apol.Feb 9 2018, 6:37 PM
apol added inline comments.
95 ↗(On Diff #26838)

This comment is very confusing, especially considering that the order you used there isn't the same as the ones below.

I'd just remove it all.

markg updated this revision to Diff 26839.Feb 9 2018, 6:39 PM

Remove comment about compiler generated functions.

markg marked an inline comment as done.Feb 9 2018, 6:40 PM
dfaure added inline comments.Feb 10 2018, 8:10 AM

... or gdb, or perf, or with debug output in the cpp file, or ....


QFile::encodeName() is the proper way


That's rather overkill (and a wrong use of the QUrl API). You want to use QFileInfo for this.


Whenever you write QVERIFY(a==b), it should be QCOMPARE(a, b)


typo: verify


same typo


swap it around: what you test on the left, what you expect on the right. This makes error messages more readable. (same above, of course)

markg updated this revision to Diff 26868.Feb 10 2018, 1:12 PM

Update test.

markg marked 7 inline comments as done.Feb 10 2018, 1:14 PM

Seems my unittest skills have become a bit rusty over the years of basically not contributing patches to KDE ;)
Thank you for the comments, David!

markg added a comment.Feb 10 2018, 1:24 PM

@dfaure i can make the test much more complete if you want.. But it's more complex thus i left it out.
I can store the UDSEntry in a QDataStream which would be backed by a QByteArray. That can be hashed!
Then i can do the same after moving operations and compare the hash. That way i would know for sure the UDSEntry objects match perfectly.

But as said, that is more complex. I'd probably make a lambda within the testMove() function. I think with a signature like this:
auto udsEntryHash = [](const KIO::UDSEntry &entry){
// some foo..
return hash; (would be the QCryptographicHash::result() output).

Your call :)

Or i can add that as a separate test that just tests UDSEntry copy and move operations. That might actually be better and more generic.
Do note that this doesn't fix or catch anything at the moment. It would just be an extra test to guarantee copying of UDSEntry objects is not broken.

dfaure requested changes to this revision.Feb 11 2018, 8:16 PM

I actually see little point in verifying that *compiler generated* code copies all members correctly. We can trust the compiler :-)

So, to me this looks ok (minus the last two small comments).

If however you were to implement a more complete checking of fields, I would recommend against hashing (how do you debug it when the expected value is wrong?), and to go instead for a string representation (concatenate data using a separator [doesn't matter if the separator is used in one of the values, there's no splitting back needed], QCOMPARE the two strings, then failures *can* be debugged). But as I said, this doesn't seem necessary here.

227 ↗(On Diff #26838)

typo: it's debugging, with an 'e'

233 ↗(On Diff #26838)

That one, however, could be QVERIFY(), that's what it's for ;)

This revision now requires changes to proceed.Feb 11 2018, 8:16 PM
markg updated this revision to Diff 27028.Feb 12 2018, 10:38 PM

Fix a typo and replace QCOMPARE with QVERIFY for the file open check.

markg marked 2 inline comments as done.Feb 12 2018, 10:45 PM

I'm not afraid of the compiler generating wrong code. That is the least of my worries :)
What i am afraid of (which is why i suggested the copy test) is a wrong optimization at some point (like adding a pointer member but forgetting to properly copy it) which would break the UDSEntry.
But a test for that would probably deserves its own review independent of this.

Forget it, i have no plans to change the internal data structure of UDSEntry. I might play with a HAT-trie at some point but that is unlikely to be on the UDSEntry level. It would be much more suitable on the KCoreDirLister level.

dfaure requested changes to this revision.Feb 13 2018, 8:44 AM

Sorry, just spotted one last typo (and two missing @since), feel free to push after fixing.

231 ↗(On Diff #26838)

Typo: tempora*r*y

94 ↗(On Diff #26838)

@since 5.44

104 ↗(On Diff #26838)

@since 5.44

This revision now requires changes to proceed.Feb 13 2018, 8:44 AM
markg added a comment.Feb 13 2018, 9:12 AM

Ahh, will fix that when i get home.
I missed the @since lines in KFileITem as well. Is it OK if i just add them outside of phabricator? They will get an "@since 5.43" i think.

markg updated this revision to Diff 27431.Feb 17 2018, 10:17 PM

Add @since lines where for the move assignment operator and constructor.
Typo fix.

markg marked 3 inline comments as done.Feb 17 2018, 10:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 17 2018, 10:19 PM
This revision was automatically updated to reflect the committed changes.

Right, this one is my fault:
Weird that QT_LSTAT is no problem on linux without including qplatformdefs.h, on windows it apparently is.
I pushed the fix:

Right, it didn't fix it...
Could someone with a windows setup (compiler and kio) compile kio and tell me what is wrong here?

Looks like QT_LSTAT doesn't exist on Windows - see

markg added a comment.EditedFeb 18 2018, 12:25 PM

Looks like QT_LSTAT doesn't exist on Windows - see

Would including "kioglobal_p.h" perhaps fix this?
It contains a define for QT_LSTAT when it doesn't exist:

#ifndef QT_LSTAT
#define QT_LSTAT kio_windows_lstat

It's a private header, but it's just for the unit tests. That would probably be fine.

If someone could test that on a windows setup?

markg added a comment.Feb 19 2018, 9:05 AM

@bcooksley Ah, i see you just fixed this by using kiotesthelper.h (which includes kioglobal_p.h).
According to Jenkins, all is OK now :)

Thank you very much!

dfaure added inline comments.Feb 25 2018, 8:02 PM

The comment is marked as done, but the code that was pushed still uses QUrl.


Same here, this was not fixed in the commit that was pushed.

markg added a comment.Feb 25 2018, 8:16 PM

I seem to be making a mess of this commit. Sorry for that. with the changes.

Regarding arc to push a change back here again, how do you do that for something that is already committed?

Uploading a new diff is the right thing to do in this case, however that won't be truly representative of what was landed in the end unless this is first reverted.
(Objective being the complete, updated diff being shown here)