<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<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/D7750" 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/D7750#inline-32572" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mgallien</span> wrote in <span style="color: #4b4d51; font-weight: bold;">extractor.cpp:34</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If the Extractor is built using the move constructor, the other instance will have a null d pointer. As far as I know, this is standard practice.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You are correct. However, to improve readability, move (haha) the implementation of the move constructor to be with the default constructor, i.e. above this freeExtractorPlugin method. This will make it clearer that there are multiple ctors, when reading top to bottom.</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/D7750#inline-32915" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">extractor.cpp:36</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 style="color: #aa4000">if</span> <span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_pluginLoader</span><span class="p">.</span><span class="n">isLoaded</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_pluginLoader</span><span class="p">.</span><span class="n">unload</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Are you sure you want to call unload? We've had many many problems in the past with unloading plugins.<br />
Anything that still refers to a static object from the plugin (e.g. QMetaObject registration, or anything else) will crash.</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/D7750#inline-32904" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">extractor.h:36</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 style="color: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Extractor</span><span class="p">(</span><span class="n">Extractor</span> <span style="color: #aa2211">&&</span><span class="n">other</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">add <tt style="background: #ebebeb; font-size: 13px;">noexcept</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/D7750#inline-32903" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">extractor.h:38</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 style="color: #aa4000">void</span> <span class="n">operator</span> <span style="color: #aa2211">=</span><span class="p">(</span><span class="n">Extractor</span> <span style="color: #aa2211">&&</span><span class="n">other</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The proper syntax for a move constructor is<br />
<tt style="background: #ebebeb; font-size: 13px;"> Extractor& operator=(Extractor &&other) noexcept</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/D7750#inline-32913" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">extractorcollection.cpp:111</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">QPluginLoader</span></span><span class="bright"> </span><span class="n"><span class="bright">loader</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">pluginPath</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">Extractor</span></span><span class="bright"> </span><span class="n"><span class="bright">newExtractor</span></span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">newExtractor</span><span class="p">.</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_pluginLoader</span><span class="p">.</span><span class="n">setFileName</span><span class="p">(</span><span class="n">pluginPath</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This lacks encapsulation. Add a setPluginFileName method to the Extract class, so d doesn't have to be public.</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/D7750#inline-32914" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">extractorcollection.cpp:123</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 style="color: #aa4000">if</span> <span class="p">(</span><span class="n">plugin</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">Extractor</span><span style="color: #aa2211">*</span> <span class="n">ex</span><span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">Extractor</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Extractor</span><span style="color: #aa2211">*</span> <span class="n">ex</span><span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">Extractor<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">std</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">move</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">newExtractor</span></span><span class="bright"></span><span class="p"><span class="bright">))</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">ex</span><span style="color: #aa2211">-></span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_plugin</span> <span style="color: #aa2211">=</span> <span class="n">plugin</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This whole std::move business is fun, but we could just add an Extractor ctor that takes an ExtractorPlugin as parameter, no?</p>
<p style="padding: 0; margin: 8px;">(and possibly a QPluginLoader, depending on what we decide about unloading).</p>
<p style="padding: 0; margin: 8px;">Then the loading will be using QPluginLoader here, as before, without a need for cloning the extractor, which looks strange to me, design wise.</p>
<p style="padding: 0; margin: 8px;">Either move the whole loading code to Extractor, or load here and then instanciate (as before), but creating two extractor instances (and moving one to the other) looks weird, overkill, and recipe for trouble.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R286 KFileMetaData</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7750" rel="noreferrer">https://phabricator.kde.org/D7750</a></div></div><br /><div><strong>To: </strong>mgallien, Frameworks, dfaure<br /><strong>Cc: </strong>dfaure, anthonyfieroni<br /></div>