<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/D9290" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Thanks!</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/D9290#inline-42119" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">helpers.h:23</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: #304a96">#ifndef KIO_HELPERS_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#define KIO_HELPERS_H</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This file should be called *_p.h to make it clear that it's not installed (and therefore not public API).</p>

<p style="padding: 0; margin: 8px;">helpers is too generic though, let's at least call this something like pathhelpers_p.h</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/D9290#inline-42117" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">helpers.h:30</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: #304a96">inline</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">QString</span> <span style="color: #004012">addPath</span><span class="p">(</span><span class="n">QString</span> <span class="n">path</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 QString & to avoid one copy</p>

<p style="padding: 0; margin: 8px;">I think the template below could also take const QString &T as input.</p>

<p style="padding: 0; margin: 8px;">I'm also wondering about the name. The name addPath made sense in KUrl, but here we're not "adding one path to an object". This is more about concatenating paths. How about concatPaths() ?</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/D9290#inline-42120" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">helpers.h:35</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">template</span> <span style="color: #aa2211"><</span><span class="n">class</span> <span class="n">T</span><span class="p">,</span> <span class="n">class</span> <span class="p">...</span><span class="n">T1</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">          <span class="n">class</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">typename</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">enable_if</span><span style="color: #aa2211"><</span><span class="n">std</span><span style="color: #aa2211">::</span><span class="n">is_same</span><span style="color: #aa2211"><</span><span class="n">T</span><span class="p">,</span> <span class="n">QString</span><span style="color: #aa2211">>::</span><span class="n">value</span><span style="color: #aa2211">>::</span><span class="n">type</span><span style="color: #aa2211">></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Fancy variadic template, but is it actually called with more than 2 args anywhere? :-)</p>

<p style="padding: 0; margin: 8px;">If not I'd suggest to keep it a simple function with 2 args.</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/D9290#inline-42118" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">helpers.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; background: rgba(151, 234, 151, .6);"><span class="n">template</span> <span style="color: #aa2211"><</span><span class="n">class</span> <span class="n">T</span><span class="p">,</span> <span class="n">class</span> <span class="p">...</span><span class="n">T1</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">          <span class="n">class</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">typename</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">enable_if</span><span style="color: #aa2211"><</span><span class="n">std</span><span style="color: #aa2211">::</span><span class="n">is_same</span><span style="color: #aa2211"><</span><span class="n">T</span><span class="p">,</span> <span class="n">QString</span><span style="color: #aa2211">>::</span><span class="n">value</span><span style="color: #aa2211">>::</span><span class="n">type</span><span style="color: #aa2211">></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">T</span> <span class="n">addPath</span><span class="p">(</span><span class="n">T</span> <span class="n">path</span><span class="p">,</span> <span class="n">T1</span><span class="p">...</span> <span class="n">pathN</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">is the enable_if needed? After all if someone calls this with int or whatever else, it will simply fail to compile, right?</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/D9290" rel="noreferrer">https://phabricator.kde.org/D9290</a></div></div><br /><div><strong>To: </strong>anthonyfieroni, Frameworks, dfaure, hein, aacid<br /></div>