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

[Ben Cooksley]

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 DETAIL
  https://phabricator.kde.org/D10414

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh

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

[Mark Gaiser]

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
  https://phabricator.kde.org/D10414

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh, kmorwinski

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

[David Faure]

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 udsentrytest.cpp: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)

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10414

To: markg, dfaure
Cc: bcooksley, apol, #frameworks, michaelh, kmorwinski

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

[Mark Gaiser]

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: bcooksley, apol, #frameworks, michaelh

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

[Mark Gaiser]

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

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.

[Ben Cooksley]

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.

[Mark Gaiser]

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
  https://phabricator.kde.org/D10414

To: markg, dfaure
Cc: apol, #frameworks, michaelh

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

[Mark Gaiser]

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: 
https://cgit.kde.org/kio.git/commit/?id=11051f3709cdc651a68990a9c17a69c33d5812db

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.

[Mark Gaiser]

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 LAST UPDATE
  https://phabricator.kde.org/D10414?vs=27431&id=27432

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

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

[Mark Gaiser]

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.

[Mark Gaiser]

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&id=27431

BRANCH
  udsentry_move

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

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

[David Faure]

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.

[Mark Gaiser]

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, dfaure
Cc: apol, #frameworks, michaelh

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

[David Faure]

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 to make a UDSEntry further down.
> +QTemporaryFile file;

Typo: tempora*r*y

> udsentry.h:94
> +/**
> + * Move constructor
> + */

@since 5.44

> udsentry.h:104
> +/**
> + * Move assignment
> + */

@since 5.44

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.

[Mark Gaiser]

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 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.

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.

[Mark Gaiser]

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&id=27028

BRANCH
  udsentry_reductions

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh

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

[David Faure]

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 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.

INLINE COMMENTS

> udsentrytest.cpp:227
> +/**
> + * Test to verify that move semantics work. This is only useful when ran 
> through a profiling or dubugging tool.
> + */

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

> udsentrytest.cpp:233
> +QTemporaryFile file;
> +QCOMPARE(file.open(), true);
> +const QByteArray filePath = QFile::encodeName(file.fileName());

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

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.

[Mark Gaiser]

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 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.

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.

[Mark Gaiser]

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: markg, dfaure
Cc: apol, #frameworks, michaelh, ngraham

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

[Mark Gaiser]

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&id=26868

BRANCH
  udsentry_reductions

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh, ngraham

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

[David Faure]

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
> +QVERIFY(file.open());
> +const QByteArray filePath = file.fileName().toLocal8Bit();
> +const QString fileName = QUrl(file.fileName()).fileName(); // 
> QTemporaryFile::fileName returns the full path.

QFile::encodeName() is the proper way

> udsentrytest.cpp:235
> +const QByteArray filePath = file.fileName().toLocal8Bit();
> +const QString fileName = QUrl(file.fileName()).fileName(); // 
> QTemporaryFile::fileName returns the full path.
> +QVERIFY(!fileName.isEmpty());

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

> udsentrytest.cpp:240
> +QT_STATBUF statBuf;
> +QVERIFY(QT_LSTAT(filePath.constData(), &statBuf) == 0);
> +KIO::UDSEntry entry(statBuf, fileName);

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

> udsentrytest.cpp:256
> +
> +// And veryfy that this works.
> +QCOMPARE(fileName, movedEntry.stringValue(KIO::UDSEntry::UDS_NAME));

typo: verify

> udsentrytest.cpp:268
> +
> +// And veryfy that this works.
> +QCOMPARE(fileName, movedEntry.stringValue(KIO::UDSEntry::UDS_NAME));

same typo

> udsentrytest.cpp:269
> +// And veryfy that this works.
> +QCOMPARE(fileName, movedEntry.stringValue(KIO::UDSEntry::UDS_NAME));
> +}

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)

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.

[Mark Gaiser]

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.

[Mark Gaiser]

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&id=26839

BRANCH
  udsentry_move

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: apol, #frameworks, michaelh, ngraham

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

[Aleix Pol Gonzalez]

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.

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.

[Mark Gaiser]

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 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10414

To: markg, dfaure
Cc: #frameworks, michaelh, ngraham

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

[Mark Gaiser]

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&id=26838

BRANCH
  udsentry_move

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: #frameworks, michaelh, ngraham

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

[Mark Gaiser]

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 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.

REPOSITORY
  R241 KIO

BRANCH
  udsentry_move

REVISION DETAIL
  https://phabricator.kde.org/D10414

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: markg, dfaure
Cc: #frameworks, michaelh, ngraham