D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a comment.


  I've been able to reproduce the bug:
  
    #10 0x00007f44d257ae1d in qt_assert (assertion=assertion at entry=0x7f44d456fc43 "it != dirItem->lstItems.end()", file=file at entry=0x7f44d456fc10 "/g/5kde/frameworks/kio/src/core/kcoredirlister_p.h", line=line at entry=308) at ../../include/QtCore/../../src/corelib/global/qlogging.h:91
    #11 0x00007f44d4535c7a in KCoreDirListerCache::reinsert (this=this at entry=0x7f44d45a5440 <(anonymous namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, item=..., oldUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister_p.h:310
    #12 0x00007f44d452825c in KCoreDirListerCache::renameDir (this=this at entry=0x7f44d45a5440 <(anonymous namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, oldUrl=..., newUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:1584
    #13 0x00007f44d452aa29 in KCoreDirListerCache::slotFileRenamed (this=0x7f44d45a5440 <(anonymous namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, _src=..., _dst=..., dstPath=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:969
  
  Let's see if this assert/crash can be avoided without reverting all the patch. Any help is welcome.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:963
> Where did the old code call refresh() (which you now call inside findByUrl)? I don't see it, I only see more specific calls in more specific cases. So this looks slower and possibly incorrect (for non-local-files).

Changed to use the reinsert semantic.

> dfaure wrote in kcoredirlister.cpp:1004
> This used to modify the fileitem in dirItem->lstItems, now it's modifying a copy.
> Same for all other fileitem.setFoo calls below.
> 
> Is there a call to reinsert missing?

Thanks, reinsert has a better semantics than refresh.

> markg wrote in kcoredirlister.cpp:825-829
> Hmm, this looks weird to me.
> Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.
> 
> A more efficient way (but requires you to change the lists this is backed by to a std::vector) is to:
> 
> 1. Take the element out of the vector. Something like "KFileItem item = std::move(*it);
> 2. Now the vector would be in a valid state but with one invalid object (will post a problem if you iterate over it later on) so you have to remove that element from the vector like you did.
> 
> I'm not sure if the benefits of this justify the path of changing the list (dirItem->lstItems) to a std::vector, if possible at all.
> That's up to you :)

Changed, using the reinsert semantic.

> bruns wrote in kcoredirlister_p.h:302
> This can be better implemented with std::move and std::move_backward from <algorithm>
> http://www.cplusplus.com/reference/algorithm/move/
> 
> 1. calculate start and end iterators from the old and new URL
> 2. move old item to tmp
> 3.
> 
>   if (old_it < new_it) {
>      std::move(old_it + 1, new_it, old_it); // move downwards
>   } else {
>      std::move_backward(new_it , old_it - 1, old_it);
>   }
>   *new_it = tmp;

I've looked at those methods, they create a new "undefined" status in the list. I don't want to worry about this new status.

> bruns wrote in kfileitem.cpp:180
> You can probably leave this out if you use the following for ordering:
> 
>   bool operator< (const KFileItem& other) {
>     if m_url.size() != other.m_url.size() {
>       return m_url.size() < other.m_url.size()
>     }
>     return m_url < other.m_url;
>   }
> 
> You don't need lexicographical sorting, but just a total ordering for the lookup.

I've tried something similar. As QUrl doesn't have a size method, I've tried with path().size(). With findByUrl it is no problem, it gives me 28 msecs vs. 1795 msecs, but inserting in the list gives me 37 msecs vs. the original 19 msecs. 
Only checking m_url < other.m_url (without m_hash) gives me 21 msecs inserting and 15 msecs in findByUrl, faster than using m_hash with the right checks for collisions (22-23 msecs inserting).

> bruns wrote in kfileitem.h:490
> The description does not match the implementation ...

Changed, thanks. Now they match.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, dfaure
Cc: elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181215/fbb1e6c6/attachment.html>