<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<br />This revision now requires changes to proceed.
</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/D13627">View Revision</a></tr></table><br /><div><div><p>You fixed the small details I found, but the big picture is still very wrong.<br />
Using the same KIconLoader in multiple threads cannot work and doesn't make sense: one thread could addAppDir(), or reinit, messing up results for another thread. <br />
Use a different KIconLoader per thread, and then the only thing you have to make threadsafe is any use of a global object, but not the kiconloader itself, which CANNOT be made threadsafe given its API (theme() is a perfect example).</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/D13627#inline-71623">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kiconeffect.cpp:74</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">KIconEffect</span><span style="color: #aa2211">&</span> <span class="n">KIconEffect</span><span style="color: #aa2211">::</span><span style="color: #aa4000">operator</span><span style="color: #aa2211">=</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">KIconEffect</span> <span style="color: #aa2211">&</span><span class="n">other</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">unrelated to this patch, right?</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/D13627#inline-71624">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kiconeffect.cpp:76</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">memcpy</span><span class="p">(</span><span class="n">d</span><span class="p">,</span> <span class="n">other</span><span class="p">.</span><span class="n">d</span><span class="p">,</span> <span style="color: #aa4000">sizeof</span><span class="p">(</span><span style="color: #aa2211">*</span><span class="n">d</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">return</span> <span style="color: #aa2211">*</span><span style="color: #aa4000">this</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">warning, very wrong if there's a 'q' pointer...</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/D13627#inline-71625">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kiconloader.cpp:1327</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QPixmap</span> <span class="n">pixmap</span> <span style="color: #aa2211">=</span> <span class="n">S_D</span><span class="p">(</span><span class="n">d</span><span class="p">)</span><span style="color: #aa2211">-></span><span class="n">loadMimeTypeIcon</span><span class="p">(</span><span class="n">_iconName</span><span class="p">,</span> <span class="n">group</span><span class="p">,</span> <span class="n">size</span><span class="p">,</span> <span class="n">state</span><span class="p">,</span> <span class="n">overlays</span><span class="p">,</span> <span class="n">path_store</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">return</span> <span class="n">pixmap</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">And now such things could be written in a much more standard and readable way with a QMutexLocker at the beginning of the method, rather then the S_D macro and proxy hack.</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/D13627#inline-71627">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kiconloader.cpp:1655</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">init</span>IconTheme<span class="bright">s</span></span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">mpT</span>heme<span class="bright">Root</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="bright"></span><span class="n"><span class="bright">K</span>IconTheme<span class="bright"></span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"></span><span class="n"><span class="bright">theme</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">S_D</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">theme</span></span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="bright"></span><span style="color: #aa4000"><span class="bright">return</span></span><span class="bright"> </span><span class="n"><span class="bright">t</span>heme<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is not threadsafe, it's wishful-thinking-threadsafe.<br />
Sure, the call to theme() is protected from interference from other threads, but if another thread can recreate the themes, then the pointer you got out of this method call is now invalid.</p>

<p style="padding: 0; margin: 8px;">So I'll say it again: use a different KIconLoader instance in every thread, MUCH simpler, MUCH safer.</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/D13627#inline-71622">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kiconloader.h:482</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright">    </span><span class="n"><span class="bright">KIconEffect</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"></span><span class="n"><span class="bright">iconEffect</span></span><span class="bright"></span><span class="p"><span class="bright">()</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"></span><span style="color: #304a96"><span class="bright">#ifndef KICONTHEMES_NO_DEPRECATED</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">KICONTHEMES_DEPRECATED</span> <span class="n">KIconEffect</span> <span style="color: #aa2211">*</span><span class="n">iconEffect</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#endif</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">unrelated to this patch, please split this out</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R302 KIconThemes</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D13627">https://phabricator.kde.org/D13627</a></div></div><br /><div><strong>To: </strong>anthonyfieroni, davidedmundson, dfaure, Frameworks<br /><strong>Cc: </strong>kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>