<table><tr><td style="">asemke 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/D14523">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/D14523#inline-76688">View Inline</a><span style="color: #4b4d51; font-weight: bold;">fkristof</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:612</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'd say it would be safer to have 2 separate ifs, even if we know that the order is left to right, if somebody changes/adds here something and the fileName.at(0) will be before the isEmpty() then there will be a crash.</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (!fileName.isEmpty()) {
       if (fileName.at(0) != QDir::separator()) {</pre></div>

</div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This part Christoph copied simply from another place... This logic we have in many places in the code where we striclty rely on the fixed ordering of the evaluation which has also to be guaranteed by the compiler. Many people are used to this and I think this is ok...</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/D14523#inline-76682">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:624</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You meant <tt style="background: #ebebeb; font-size: 13px;">const QString fileName</tt>, right?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I meant const reference. Though the difference with respect to the performance between copying an object and creating a reference to it might be very small for implicitly shared Qt classes (QString is implicitely shared), it's still matter of convention and of code styly. With const QString& you _explicitly_ say I do not want to copy and I'm not going to change the reference - this is what we try to consistenly use throughout the code.</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/D14523#inline-76678">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.cpp:843</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Cannot make it const, it's modified later on.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">ok.</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/D14523#inline-76684">View Inline</a><span style="color: #4b4d51; font-weight: bold;">croick</span> wrote in <span style="color: #4b4d51; font-weight: bold;">ImportFileWidget.h:111</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">No, but there were only slots so far (also unconnected).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok. We should clean up here a bit maybe later and make private functions. Thanks for the pointer.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R262 LabPlot</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14523">https://phabricator.kde.org/D14523</a></div></div><br /><div><strong>To: </strong>croick, LabPlot<br /><strong>Cc: </strong>fkristof, asemke, kde-edu, narvaez, apol<br /></div>