Adds move semantics support and a testcase (only useful in callgrind).
Right now there are 0 uses of these semantics.
Details
- Reviewers
dfaure - Commits
- R241:d8414425acc5: Add move semantics support to KIO::UDSEntry.
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
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.
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. |
autotests/udsentrytest.cpp | ||
---|---|---|
227 ↗ | (On Diff #26836) | ... or gdb, or perf, or with debug output in the cpp file, or .... |
234 ↗ | (On Diff #26836) | QFile::encodeName() is the proper way |
235 ↗ | (On Diff #26836) | That's rather overkill (and a wrong use of the QUrl API). You want to use QFileInfo for this. |
240 ↗ | (On Diff #26836) | Whenever you write QVERIFY(a==b), it should be QCOMPARE(a, b) |
256 ↗ | (On Diff #26836) | typo: verify |
268 ↗ | (On Diff #26836) | same typo |
269 ↗ | (On Diff #26836) | 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) |
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!
@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.
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 ↗ | (On Diff #26836) | typo: it's debugging, with an 'e' |
233 ↗ | (On Diff #26836) | That one, however, could be QVERIFY(), that's what it's for ;) |
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.
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.
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/
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?
@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!
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)