D13211: Enable comparing KFileItems by url
jtamate added dependent revisions: D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache, D12945: kcoredirlister lstItems benchmark. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 To: jtamate, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13211: Enable comparing KFileItems by url
jtamate created this revision. jtamate added a reviewer: dfaure. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY Based on QUrl comparisons (available since Qt 5.4). Those comparisons can be used to speedup the lookup of the requested KFileItem. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13211: Enable comparing KFileItems by url
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kfileitem.cpp:1241
> +{
> +if (!d || !other.d) {
> +return false;
This isn't symmetric. operator< must have the property that a kfileitem.h:493
> + */
> +bool operator<(const KFileItem &other) const;
> +
This one looks good.
> kfileitem.h:499
> + */
> +bool operator<(const QUrl &other) const;
> +
This operator seems strange to me, it's comparing a KFileItem and a QUrl, which
are two different kinds of objects... What's the use case?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D13211
To: jtamate, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13211: Enable comparing KFileItems by url
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. A good example of how a unittest helps catching a bug :-) (and a good example of how code that I suggest isn't necessarily bugfree, haha) Missing: unittests for the < QUrl overload. The case of the invalid item vs an invalid URL will be interesting too ;) INLINE COMMENTS > kfileitemtest.cpp:305 > +// a KFileItem without url is considered < than anything. > +QVERIFY(nulFileItem < nulFileItem); > +QVERIFY(nulFileItem < fileItem); I wonder if that should be true, though ;-) I guess it should rather be false, so that a binary search (based on < only) can deduce that a null item is *equal* to another null item, rather than both being lower than the other, which would be nonsense. > kfileitemtest.cpp:314 > +QVERIFY(!(fileItem3 < fileItem)); > +// This should be false as they are equal > +QVERIFY(!(fileItem < fileItem)); ... exactly. I think the same reasoning applies to a null item. > kfileitem.h:490 > +/** > + * Returns true if other's URL is lexically greater than this url; > otherwise returns false > + * @since 5.47 "this item's URL" > kfileitem.h:496 > +/** > + * Returns true if url other is lexically greater than this url; > otherwise returns false > + * @since 5.47 "than this item's URL", then REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 To: jtamate, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13211: Enable comparing KFileItems by url
bruns added inline comments. INLINE COMMENTS > dfaure wrote in kfileitem.cpp:1241 > This isn't symmetric. operator< must have the property that a aren't both true, and that they are both false only if the items are equal. > > If `this` is a valid item and `other` is null, then this code says that > *this Which would let some algorithms deduce that *this == other, which is > completely wrong. > > If we decide that a null item is inferior to anything else, then we need > > if (!d) return true; if (!other.d) return false; > > (with newlines and braces of course). > > This also calls for a corresponding unittest. > If we decide that a null item is inferior to anything else, then we need > if (!d) return true; if (!other.d) return false; this has to be `if (!other.d) return false; if (!d) return true;` (mind the order) otherwise (nullptr) < (nullptr). REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
jtamate updated this revision to Diff 35188. jtamate marked 3 inline comments as done. jtamate edited the test plan for this revision. jtamate added a comment. A KFileItem without url will be the lowest, even lower than itself. Created a new test. The comparison with the QUrl is for this use case: auto it = std::lower_bound(dirItem->lstItems.begin(), dirItem->lstItems.end(), oldUrl); REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=35180&id=35188 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13211: Enable comparing KFileItems by url
jtamate updated this revision to Diff 35190. jtamate marked 4 inline comments as done. jtamate added a comment. Invalid items are not less than invalid items or invalid urls, they are not like -infinite. Added the tests comparing items with urls. Changed the descriptions. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=35188&id=35190 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
jtamate updated this revision to Diff 35192. jtamate added a comment. Taken into account invalid Items created from invalid QUrls. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=35190&id=35192 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
jtamate updated this revision to Diff 35303. jtamate added a comment. Now passes the tests and its performance for non invalid items is not degraded too much (same +3ms inserting). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=35192&id=35303 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
jtamate updated this revision to Diff 36037. jtamate marked an inline comment as done. jtamate edited the summary of this revision. jtamate added a comment. Change @since to 5.48 REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=35303&id=36037 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
This revision was automatically updated to reflect the committed changes. Closed by commit R241:fb3c94ed96c3: Enable comparing KFileItems by url (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13211?vs=36037&id=36112 REVISION DETAIL https://phabricator.kde.org/D13211 AFFECTED FILES autotests/kfileitemtest.cpp autotests/kfileitemtest.h src/core/kfileitem.cpp src/core/kfileitem.h To: jtamate, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13211: Enable comparing KFileItems by url
markg added a comment. While this works, there is a newer en better way for it. It's new in C++14 and called "transparent compare". In "this" case it won't change the resulting code of how you compare, but it might be worth checking that out. Read this: https://www.fluentcpp.com/2017/06/09/search-set-another-type-key/ And watch this: https://www.youtube.com/watch?v=BBUacofxOP8 those will explain how to use it. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13211 To: jtamate, dfaure Cc: markg, bruns, kde-frameworks-devel, michaelh, ngraham
