<table><tr><td style="">jtamate added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D10742">View Revision</a></tr></table><br /><div><div><p>I've been able to reproduce the bug:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#10 0x00007f44d257ae1d in qt_assert (assertion=assertion@entry=0x7f44d456fc43 "it != dirItem->lstItems.end()", file=file@entry=0x7f44d456fc10 "/g/5kde/frameworks/kio/src/core/kcoredirlister_p.h", line=line@entry=308) at ../../include/QtCore/../../src/corelib/global/qlogging.h:91
#11 0x00007f44d4535c7a in KCoreDirListerCache::reinsert (this=this@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@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</pre></div>

<p>Let's see if this assert/crash can be avoided without reverting all the patch. Any help is welcome.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-68210">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kcoredirlister.cpp:963</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changed to use the reinsert semantic.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-68209">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kcoredirlister.cpp:1004</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This used to modify the fileitem in dirItem->lstItems, now it's modifying a copy.<br />
Same for all other fileitem.setFoo calls below.</p>

<p style="padding: 0; margin: 8px;">Is there a call to reinsert missing?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Thanks, reinsert has a better semantics than refresh.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-51273">View Inline</a><span style="color: #4b4d51; font-weight: bold;">markg</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kcoredirlister.cpp:825-829</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Hmm, this looks weird to me.<br />
Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.</p>

<p style="padding: 0; margin: 8px;">A more efficient way (but requires you to change the lists this is backed by to a std::vector) is to:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">Take the element out of the vector. Something like "KFileItem item = std::move(*it);</li>
<li class="remarkup-list-item">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.</li>
</ol>

<p style="padding: 0; margin: 8px;">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.<br />
That's up to you :)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changed, using the reinsert semantic.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-67582">View Inline</a><span style="color: #4b4d51; font-weight: bold;">bruns</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kcoredirlister_p.h:302</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This can be better implemented with std::move and std::move_backward from <algorithm><br />
<a href="http://www.cplusplus.com/reference/algorithm/move/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.cplusplus.com/reference/algorithm/move/</a></p>

<ol class="remarkup-list">
<li class="remarkup-list-item">calculate start and end iterators from the old and new URL</li>
<li class="remarkup-list-item">move old item to tmp</li>
<li class="remarkup-list-item"></li>
</ol>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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;</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-67584">View Inline</a><span style="color: #4b4d51; font-weight: bold;">bruns</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kfileitem.cpp:180</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You can probably leave this out if you use the following for ordering:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">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;
}</pre></div>

<p style="padding: 0; margin: 8px;">You don't need lexicographical sorting, but just a total ordering for the lookup.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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. <br />
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).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D10742#inline-67576">View Inline</a><span style="color: #4b4d51; font-weight: bold;">bruns</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kfileitem.h:490</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The description does not match the implementation ...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changed, thanks. Now they match.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10742">https://phabricator.kde.org/D10742</a></div></div><br /><div><strong>To: </strong>jtamate, Frameworks, dfaure<br /><strong>Cc: </strong>elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham<br /></div>