<table><tr><td style="">mwolff added a comment.
</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/D17289">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D17289#401264" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D17289#401264</a>, <a href="https://phabricator.kde.org/p/rjvbb/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rjvbb</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><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);">we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse</pre></div></blockquote>
<p>Shall we keep that discussion to <a href="https://phabricator.kde.org/D18551" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D18551</a>?</p></div>
</blockquote>
<p>Yes.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><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);">that's ~50ish code locations... Quite a lot :-/</pre></div></blockquote>
<p>Erm, yes, so many that it begs the question if "this is worth it"... Is it?</p></blockquote>
<p>Exactly.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>I only looked at places where the existing env. profile feature is used. There were already enough of those that I didn't feel like changing them all potentially for nothing, but I don't think there were this many.</p>
<p>Why would we not limit the change to those locations, or in other words, why is the selected env. profile not used for every process KDevelop spawns?</p></blockquote>
<p>Because someone needs to write the code for that. And there are probably places where the simpler QProcess API was used instead. One way or another, someone needs to refactor all of the code.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Would it be feasible to reduce the footprint of this overhaul by using a wrapper around writableLocation(QSP::TempLocation) instead?</p></blockquote>
<p>I don't see how that would change anything, considering we need to change TMPDIR for libclang, which isn't using Qt internally?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>I'd have to check in how many locations libclang reads TMPDIR and so if it's feasible to change that variable temporarily around the parser's entrypoints into libclang.</p></blockquote>
<p>You cannot, since env vars are process-wide and thus changing them from a background thread would lead to race conditions.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>(replying by email)</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>+#if defined(Q_OS_UNIX)<br />
+ auto tmpLocation = QStandardPaths::writableLocation(QStandardPaths::TempLocation);</p></blockquote>
<p>imo this should also be handled on windows via <tt style="background: #ebebeb; font-size: 13px;">TMP</tt> and <tt style="background: #ebebeb; font-size: 13px;">TEMP</tt>, according to</p></blockquote>
<p>Aren't those used in the MSWin version of <tt style="background: #ebebeb; font-size: 13px;">TempLocation</tt>? I can try to implement this for MSWin too but only if someone promises to test it (I can't).</p>
<p>BTW, cf. <a href="https://codereview.qt-project.org/#/c/239952" class="remarkup-link" target="_blank" rel="noreferrer">https://codereview.qt-project.org/#/c/239952</a></p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>all of this code shouldn't be required - the system env used to launch subprocesses should not use the changed tmpdir after all, so it cannot be recursive</p></blockquote>
<p>Evidently this can go once that restored system env. is used.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>+ m_tmpDir->mkpath(tmpLocation);</p></blockquote>
<p>it exists, you create the path in the line above</p></blockquote>
<p>Except when mkpath failed because tmpLocation was messed up. KDevelop does NOT like that. That happened to me during development and I decided to leave the check in.</p></blockquote>
<p>Then handle mkpath's return value properly and error out</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>+ void systemEnvironment(QProcessEnvironment& env) const;</p></blockquote>
<p>don't use out params, just return a QProcessEnvironment similar to how it's done in <tt style="background: #ebebeb; font-size: 13px;">QProcessEnvironment::systemEnvironment()</tt></p></blockquote>
<p>This was out of a concern that certain spawned processes might be performance critical. I guess the overhead of the extra copy can be neglected then?</p></blockquote>
<p>the value is implicitly shared, returning it is cheap</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17289">https://phabricator.kde.org/D17289</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, kfunk, mwolff<br /><strong>Cc: </strong>aaronpuchert, mwolff, pino, kfunk, kdevelop-devel, gennad, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd<br /></div>