D10414: Add move semantics support to KIO::UDSEntry.

bcooksley added a comment. 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) REPOSITORY R241 KIO REVISION

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. 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? REPOSITORY R241 KIO REVISION DETAIL

D10414: Add move semantics support to KIO::UDSEntry.

dfaure added inline comments. INLINE COMMENTS > dfaure wrote in udsentrytest.cpp:235 > That's rather overkill (and a wrong use of the QUrl API). You want to use > QFileInfo for this. The comment is marked as done, but the code that was pushed still uses QUrl. > dfaure wrote in

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. @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! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg, dfaure Cc:

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. In D10414#208515 , @bcooksley wrote: > 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? #ifndef QT_LSTAT

D10414: Add move semantics support to KIO::UDSEntry.

bcooksley added a comment. Looks like QT_LSTAT doesn't exist on Windows - see https://git.reviewboard.kde.org/r/127727/ REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg, dfaure Cc: bcooksley, apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. 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? REPOSITORY R241 KIO REVISION DETAIL

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. 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:

D10414: Add move semantics support to KIO::UDSEntry.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R241:d8414425acc5: Add move semantics support to KIO::UDSEntry. (authored by markg). REPOSITORY R241 KIO CHANGES SINCE

D10414: Add move semantics support to KIO::UDSEntry.

markg marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg, dfaure Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

markg updated this revision to Diff 27431. markg added a comment. Add @since lines where for the move assignment operator and constructor. Typo fix. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10414?vs=27028=27431 BRANCH udsentry_move REVISION DETAIL

D10414: Add move semantics support to KIO::UDSEntry.

dfaure added a comment. Yes. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg, dfaure Cc: apol, #frameworks, michaelh

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. 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. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg,

D10414: Add move semantics support to KIO::UDSEntry.

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Sorry, just spotted one last typo (and two missing @since), feel free to push after fixing. INLINE COMMENTS > udsentrytest.cpp:231 > +{ > +// Create a temporaty file. Just

D10414: Add move semantics support to KIO::UDSEntry.

markg marked 2 inline comments as done. markg added a comment. 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

D10414: Add move semantics support to KIO::UDSEntry.

markg updated this revision to Diff 27028. markg added a comment. Fix a typo and replace QCOMPARE with QVERIFY for the file open check. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10414?vs=26868=27028 BRANCH udsentry_reductions REVISION DETAIL

D10414: Add move semantics support to KIO::UDSEntry.

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. @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

D10414: Add move semantics support to KIO::UDSEntry.

markg marked 7 inline comments as done. markg added a comment. 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! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To:

D10414: Add move semantics support to KIO::UDSEntry.

markg updated this revision to Diff 26868. markg added a comment. Update test. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10414?vs=26839=26868 BRANCH udsentry_reductions REVISION DETAIL https://phabricator.kde.org/D10414 AFFECTED FILES

D10414: Add move semantics support to KIO::UDSEntry.

dfaure added inline comments. INLINE COMMENTS > udsentrytest.cpp:227 > +/** > + * Test to verify that move semantics work. This is only useful when ran > through callgrind. > + */ ... or gdb, or perf, or with debug output in the cpp file, or > udsentrytest.cpp:234 > +

D10414: Add move semantics support to KIO::UDSEntry.

markg marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10414 To: markg, dfaure Cc: apol, #frameworks, michaelh, ngraham

D10414: Add move semantics support to KIO::UDSEntry.

markg updated this revision to Diff 26839. markg added a comment. Remove comment about compiler generated functions. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10414?vs=26838=26839 BRANCH udsentry_move REVISION DETAIL

D10414: Add move semantics support to KIO::UDSEntry.

apol added inline comments. INLINE COMMENTS > udsentry.cpp:95 > +// Default implementations for: > +// - Copy constructor > +// - Move constructor 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.

D10414: Add move semantics support to KIO::UDSEntry.

markg added a comment. 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

D10414: Add move semantics support to KIO::UDSEntry.

markg updated this revision to Diff 26838. markg added a comment. Fix indentation. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10414?vs=26836=26838 BRANCH udsentry_move REVISION DETAIL https://phabricator.kde.org/D10414 AFFECTED FILES

D10414: Add move semantics support to KIO::UDSEntry.

markg created this revision. markg added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. markg requested review of this revision. REVISION SUMMARY Adds move semantics support and a testcase (only useful in