Based on QUrl comparisons (available since Qt 5.4).
Those comparisons can be used to speedup the lookup of the requested KFileItem, for example using std::lower_bound.
Details
- Reviewers
dfaure - Commits
- R241:fb3c94ed96c3: Enable comparing KFileItems by url
Included a new test in kfileitemtest
Diff Detail
- Repository
- R241 KIO
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
src/core/kfileitem.cpp | ||
---|---|---|
1241 | This isn't symmetric. operator< must have the property that a<b and b<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<other is false and other<*this is false. 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. | |
src/core/kfileitem.h | ||
493 | This one looks good. | |
499 | 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? |
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);
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 ;)
autotests/kfileitemtest.cpp | ||
---|---|---|
305 | 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. | |
314 | ... exactly. I think the same reasoning applies to a null item. | |
src/core/kfileitem.h | ||
490 | "this item's URL" | |
496 | "than this item's URL", then |
src/core/kfileitem.cpp | ||
---|---|---|
1241 |
this has to be otherwise (nullptr) < (nullptr). |
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.
Now passes the tests and its performance for non invalid items is not degraded too much (same +3ms inserting).
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.