<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/D25682">View Revision</a></tr></table><br /><div><div><p>I guess you were expecting a higher-level review, but I don't know anything about these protocols.</p>
<p>I'm glad to see my KDSoap library being useful in KDE too though :-)</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/D25682#inline-145485">View Inline</a><span style="color: #4b4d51; font-weight: bold;">discovery.cpp:21</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; color: #888888;">(Empty.)</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not sure this .cpp file serves any purpose right now ;)</p>
<p style="padding: 0; margin: 8px;">But actually the virtual destructors could be implemented here out-of-line to avoid being generated in every user of the class. The <tt style="background: #ebebeb; font-size: 13px;">=default</tt> syntax would work here too.</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/D25682#inline-145483">View Inline</a><span style="color: #4b4d51; font-weight: bold;">discovery.h:2</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: #74777d">/*</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> Copyright 2019 Harald Sitter <sitter@kde.org</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What happened to the '>' character?</p>
<p style="padding: 0; margin: 8px;">[repeats]</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/D25682#inline-145511">View Inline</a><span style="color: #4b4d51; font-weight: bold;">discovery.h:40</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">virtual</span> <span style="color: #aa2211">~</span><span class="n">Discovery</span><span class="p">()</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">default</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">virtual</span> <span style="color: #aa4000">void</span> <span style="color: #004012">toEntry</span><span class="p">(</span><span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">UDSEntry</span> <span style="color: #aa2211">&</span><span class="n">entry</span><span class="p">)</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</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;">Why don't these <tt style="background: #ebebeb; font-size: 13px;">toEntry()</tt> methods *return* a KIO::UDSEntry instead?<br />
It's always empty on incoming anyway.</p>
<p style="padding: 0; margin: 8px;">And UDSEntry supports moving so the "return" statement won't make a copy.</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/D25682#inline-145484">View Inline</a><span style="color: #4b4d51; font-weight: bold;">discovery.h:51</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">virtual</span> <span style="color: #aa4000">void</span> <span style="color: #004012">stop</span><span class="p">()</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">virtual</span> <span style="color: #aa4000">bool</span> <span style="color: #004012">isFinished</span><span class="p">()</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">const?</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/D25682#inline-145487">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dnssddiscoverer.cpp:46</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: #74777d">// that assumption will be true all the time. ~sitter, 2018</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QUrl</span> <span style="color: #004012">u</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"smb://"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">u</span><span class="p">.</span><span class="n">setHost</span><span class="p">(</span><span class="n">m_service</span><span style="color: #aa2211">-></span><span class="n">hostName</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">u.setScheme(QStringLiteral("smb"))</tt> would be slightly faster; you're constructing a URL from its parts, no need to trigger parsing.</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/D25682#inline-145488">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dnssddiscoverer.cpp:71</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: #74777d">// RemoteService::Ptr is useless here.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">it</span> <span class="p">:</span> <span class="n">m_services</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 style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">*</span><span class="n">service</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">*</span><span class="n">it</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">qAsConst</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/D25682#inline-145489">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dnssddiscoverer.cpp:103</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">m_browser</span><span class="p">.</span><span class="n">disconnect</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">service</span> <span class="p">:</span> <span class="n">m_services</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">service</span><span style="color: #aa2211">-></span><span class="n">resolve</span><span class="p">();</span> <span style="color: #74777d">// Blocks until resolution happened. Our signal handle then jumps in.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">qAsConst</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/D25682#inline-145486">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dnssddiscoverer.h:41</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">class</span> <span style="color: #a0a000">DNSSDDiscoverer</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QObject</span><span class="p">,</span> <span class="n">public</span> <span class="n">virtual</span> <span class="n">Discoverer</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;">Why <tt style="background: #ebebeb; font-size: 13px;">virtual</tt>? I thought this only mattered for diamond-shaped inheritance.</p>
<p style="padding: 0; margin: 8px;">(OTOH I remember it leads to strange order of ctors being called, so I avoid it as much as possible)</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/D25682#inline-145490">View Inline</a><span style="color: #4b4d51; font-weight: bold;">.gitlab-ci.yml:2</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="l l-Scalar l-Scalar-Plain">fedora</span><span class="p p-Indicator">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="l l-Scalar l-Scalar-Plain">image</span><span class="p p-Indicator">:</span> <span class="l l-Scalar l-Scalar-Plain">registry.gitlab.com/caspermeijn/docker-images/fedora-build-onvifviewer:latest</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="l l-Scalar l-Scalar-Plain">stage</span><span class="p p-Indicator">:</span> <span class="l l-Scalar l-Scalar-Plain">build</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">(is this meant to go into kde git? just wondering)</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/D25682#inline-145491">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt.user:3</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);"><!DOCTYPE QtCreatorProject>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><!-- Written by QtCreator 4.9.1, 2019-08-07T17:08:05. -->
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><qtcreator>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Now this one I'm sure, should NOT go into git.</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/D25682#inline-145492">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt.user:205</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);"> </valuelist>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <value type="QString" key="ProjectExplorer.BuildConfiguration.BuildDirectory">/home/me/src/git/soapy/build-kdsoap-ws-discovery-client-Desktop-Release-with-Debug-Information</value>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <valuemap type="QVariantMap" key="ProjectExplorer.BuildConfiguration.BuildStepList.0">
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">... because it's about your config :)</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/D25682#inline-145493">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverytargetservice.cpp: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">bool</span> <span class="n">WSDiscoveryTargetService</span><span style="color: #aa2211">::</span><span class="n">isMatchingType</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">KDQName</span> <span style="color: #aa2211">&</span><span class="n">matchingType</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;">const</p>
<p style="padding: 0; margin: 8px;">(this will avoid detaching m_typeList)</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/D25682#inline-145494">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverytargetservice.cpp:48</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">bool</span> <span class="n">WSDiscoveryTargetService</span><span style="color: #aa2211">::</span><span class="n">isMatchingScope</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QUrl</span> <span style="color: #aa2211">&</span><span class="n">matchingScope</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;">same</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/D25682#inline-145495">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kio_smb_browse.cpp:479</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">normalizedUrl</span><span class="p">.</span><span class="n">path</span><span class="p">().</span><span class="n">isEmpty</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 style="color: #304a96">#pragma message "refactor this entire function into less of a long pile of madness"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qDebug</span><span class="p">()</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"Trying modern discovery (dnssd/wsdiscovery)"</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">lol</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/D25682#inline-145510">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kio_smb_browse.cpp:494</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">connect</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">sendTimer</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">QTimer</span><span style="color: #aa2211">::</span><span class="n">timeout</span><span class="p">,</span> <span style="color: #aa4000">this</span><span class="p">,</span> <span class="p">[</span><span style="color: #aa2211">&</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 style="color: #aa4000">if</span> <span class="p">(</span><span class="n">list</span><span class="p">.</span><span class="n">size</span><span class="p">()</span> <span style="color: #aa2211"><</span> <span style="color: #601200">1</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 style="color: #aa4000">return</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">isEmpty() is more readable.</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/D25682#inline-145512">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kio_smb_browse.cpp:503</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">auto</span> <span class="n">enterDiscovery</span> <span style="color: #aa2211">=</span> <span class="p">[</span><span style="color: #aa2211">&</span><span class="p">](</span><span class="n">Discovery</span><span style="color: #aa2211">::</span><span class="n">Ptr</span> <span class="n">discovery</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">discovery</span><span style="color: #aa2211">-></span><span class="n">toEntry</span><span class="p">(</span><span class="n">udsentry</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">list</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">udsentry</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">With my suggestion to return a udsentry, this whole lambda becomes</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">list.append(discovery->toEntry())</tt></p>
<p style="padding: 0; margin: 8px;">(another move, no copy)</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/D25682#inline-145513">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kio_smb_browse.cpp:511</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">QList</span><span style="color: #aa2211"><</span><span class="n">Discoverer</span> <span style="color: #aa2211">*></span> <span class="n">discoverers</span> <span class="p">{</span><span style="color: #aa2211">&</span><span class="n">d</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">w</span><span class="p">};</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">const QList.... to avoid detaching in range-for below</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/D25682#inline-145496">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kio_smb_browse.cpp:519</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">allFinished</span> <span style="color: #aa2211">&=</span> <span class="n">discoverer</span><span style="color: #aa2211">-></span><span class="n">isFinished</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;">never use &= for booleans, it's not meant for that</p>
<p style="padding: 0; margin: 8px;">(it won't work for '1' and '2' which are both 'true' for booleans)</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">allFinished = allFinished && discoverer->isFinished()</tt></p>
<p style="padding: 0; margin: 8px;">Unfortunately there's no &&= in C++.</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/D25682#inline-145507">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:116</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">qWarning</span><span class="p">()</span> <span style="color: #aa2211"><<</span> <span class="n">response</span><span class="p">.</span><span class="n">faultAsString</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// No return! We'd disqualify systems that do not implement pbsd.</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;">OK. But maybe an <tt style="background: #ebebeb; font-size: 13px;">else</tt> then ?<br />
Not much point in using response.childValues() on a fault, i.e. iterating over the fault details.</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/D25682#inline-145506">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:128</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">QString</span> <span class="n">computer</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">section</span> <span class="p">:</span> <span class="n">response</span><span class="p">.</span><span class="n">childValues</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">computer</span> <span style="color: #aa2211">=</span> <span class="n">section</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt containers.</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/D25682#inline-145505">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:160</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">const</span> <span class="n">QHostAddress</span> <span class="n">address</span><span class="p">(</span><span class="n">m_endpointUrl</span><span class="p">.</span><span class="n">toString</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qDebug</span><span class="p">()</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"++++++++++++++++++++++++++++++++++++++++++++"</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">unused</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/D25682#inline-145499">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:209</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: #74777d">// We do set a suitable timeout in the resolver so this doesn't take forever.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">future</span> <span class="p">:</span> <span class="n">m_futures</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">future</span><span class="p">.</span><span class="n">waitForFinished</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">qAsConst</p>
<p style="padding: 0; margin: 8px;">auto & ?</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/D25682#inline-145502">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:252</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">addr</span> <span style="color: #aa2211">=</span> <span class="n">xAddr</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#warning only get first val we only need one addr</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;">then rewrite this code to <tt style="background: #ebebeb; font-size: 13px;">.at(0)</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/D25682#inline-145509">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:257</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: #74777d">// Probably should just qobject this. Hardly worth the threading.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_futures</span> <span style="color: #aa2211"><<</span> <span class="n">QtConcurrent</span><span style="color: #aa2211">::</span><span class="n">run</span><span class="p">([</span><span style="color: #aa2211">=</span><span class="p">]</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">especially with the data races.</p>
<p style="padding: 0; margin: 8px;">(service->endpointReference() reads data written by another thread, without mutex)</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/D25682#inline-145504">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.cpp:280</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">entry</span><span class="p">.</span><span class="n">fastInsert</span><span class="p">(</span><span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">UDSEntry</span><span style="color: #aa2211">::</span><span class="n">UDS_ICON_NAME</span><span class="p">,</span> <span style="color: #766510">"network-server"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">entry</span><span class="p">.</span><span class="n">fastInsert</span><span class="p">(</span><span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">UDSEntry</span><span style="color: #aa2211">::</span><span class="n">UDS_ICON_OVERLAY_NAMES</span><span class="p">,</span> <span style="color: #766510">"/home/me/.local/share/icons/Windows10Icons/32x32/places/ubuntu-logo.png"</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Obviously unfinished :-)</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/D25682#inline-145498">View Inline</a><span style="color: #4b4d51; font-weight: bold;">wsdiscoverer.h:48</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">class</span> <span style="color: #a0a000">WSDiscoverer</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QObject</span><span class="p">,</span> <span class="n">public</span> <span class="n">virtual</span> <span class="n">Discoverer</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;">(same question about virtual)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R320 KIO Extras</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D25682">https://phabricator.kde.org/D25682</a></div></div><br /><div><strong>To: </strong>sitter, dfaure, Frameworks, Dolphin<br /><strong>Cc: </strong>ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, mikesomov<br /></div>