<table><tr><td style="">rjvbb added inline comments.
</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/D7995" rel="noreferrer">View Revision</a></tr></table><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/D7995#inline-33335" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.cpp:126</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">remove the conditionals, always output the time?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">fine with me, someone suggested I add the conditional ;)</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/D7995#inline-33334" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.h:106</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">actually, isn't this now unsafe API? If the dir watcher needs to be protected by a mutex, we must not hand it out as a raw pointer, no?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't think it's any more unsafe than it already was; I don't see anything that suggests that using a KDirWatch once from another thread makes it unsafe to use from its main thread afterwards. Or rather, concurrently from multiple threads because that's what the mutex protects against.<br />
Maybe David can tell us otherwise, but a priori it should be enough to add a warning in the comment that KDirWatch isn't thread-safe.</p>

<p style="padding: 0; margin: 8px;">I've tried my benchmark app without the mutex locally, and managed to reproduce the double-free crash quite regularly (on Linux). I think it would have been observed in the wild by now if the result from <tt style="background: #ebebeb; font-size: 13px;">projectWatcher</tt> was used concurrently.</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/D7995#inline-33333" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.h:111</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">this should be moved into KDirWatch, then we can use the getter above</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I agree it's lacking from KDirWatch, but can't we develop the change here first and then see what needs to be upstreamed?</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/D7995#inline-33331" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.h:114</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">so it's like closing the project, no? why do</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why do what?</p>

<p style="padding: 0; margin: 8px;">It's part of what happens when closing a project in KDevelop, but it isn't actually closing the project. It's telling the file manager to forget about the project. There is no current use case for the method in KDevelop itself, it's only for the benchmark. I tried to get the signal sent through the ProjectController (<tt style="background: #ebebeb; font-size: 13px;">projectController()->closeProject(m_project)</tt>) but that seems to need more setting up. I don't have time to figure out what and if that is going to add noise to the benchmark (parser overhead, for instance).</p>

<p style="padding: 0; margin: 8px;">The <tt style="background: #ebebeb; font-size: 13px;">detach</tt> method could be useful in a mode where you want to keep a project open for quick reference (or snatching some code snippets) only and don't want to waste resources on parsing etc.</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/D7995#inline-33337" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">filemanagerlistjob.cpp:108</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">aren't you adding the same dir twice now? i.e. once here, and once above when the dir gets listed itself?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Did I misunderstand, doesn't this loop add the contents of the folder at <tt style="background: #ebebeb; font-size: 13px;">path</tt>?</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/D7995#inline-33344" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:48</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">are you only changing visibility here? if so, use <tt style="background: #ebebeb; font-size: 13px;">using</tt> instead, or better yet - add the <tt style="background: #ebebeb; font-size: 13px;">AFMPBenchmark</tt> as friend class to the <tt style="background: #ebebeb; font-size: 13px;">AFMP</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Yes, just visibility change. I didn't think it was OK to add a (private) test class as a friend to public API. Will do that, then.</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/D7995#inline-33342" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:138</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">use a plain vector or std::array or c-array? Or are the enumerators non-contiguous? if the latter, use qhash</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The enumerator doesn't specify values; is there a standard that guarantees that such an enum always counts from 0 by 1?</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/D7995#inline-33346" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:158</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I think you misunderstood me. I don't want to keep the benchmark of the old implementation around. What I want is a simple benchmark (separate patch!) that can be run on the current implementation. Then when one applies your optimization patch, it should show a performance improvement of the benchmark.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Well, you didn't clarify that so I did what cost me the least time (for the initial implementation). I'm ok with adding a benchmark first as long as that can also go into the 5.2 branch, along with any API additions like <tt style="background: #ebebeb; font-size: 13px;">AbstractFileManagerPlugin::detach()</tt> (or however we'll be calling it).</p>

<p style="padding: 0; margin: 8px;">I was kind of hoping that everyone who actually uses the benchmark would see the obvious performance improvement when using <tt style="background: #ebebeb; font-size: 13px;">KDIRWATCH_METHOD=QFSWatch</tt> during this review process, which would remove the need to commit the BENCHMARK_ORIGINAL_DIRWATCHER feature.</p>

<p style="padding: 0; margin: 8px;">It *was* the idea to use the benchmark before committing it, no?</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/D7995#inline-33343" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:172</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">range-based for</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Sorry, what's the difference?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7995" rel="noreferrer">https://phabricator.kde.org/D7995</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, mwolff<br /><strong>Cc: </strong>aaronpuchert, arrowdodger, kfunk, dfaure, mwolff, brauch, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>