Add move semantics support to KIO::UDSEntry.
ClosedPublic

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

Details

Summary

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

Repository
R241 KIO
Branch
udsentry_move
Lint
No Linters Available
Unit
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.
src/core/udsentry.cpp
95

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
autotests/udsentrytest.cpp
227

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

234

QFile::encodeName() is the proper way

235

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

240

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

256

typo: verify

268

same typo

269

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.

autotests/udsentrytest.cpp
227

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

233

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.

autotests/udsentrytest.cpp
231

Typo: tempora*r*y

src/core/udsentry.h
94

@since 5.44

104

@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: https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20WindowsMSVCQt5.9/193/
Weird that QT_LSTAT is no problem on linux without including qplatformdefs.h, on windows it apparently is.
I pushed the fix: https://cgit.kde.org/kio.git/commit/?id=11051f3709cdc651a68990a9c17a69c33d5812db

Right, it didn't fix it... https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20WindowsMSVCQt5.9/195/console
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 https://git.reviewboard.kde.org/r/127727/

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

Looks like QT_LSTAT doesn't exist on Windows - see https://git.reviewboard.kde.org/r/127727/

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
#endif

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
autotests/udsentrytest.cpp
235

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

269

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.
https://p.sc2.nl/B13Etcldf 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)