<table><tr><td style="">rjvbb marked 3 inline comments as done.<br />rjvbb 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/D17858">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/D17858#inline-100010">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">clangsupport.cpp:188</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">the path is empty, so nothing will be printed here</p>
<p style="padding: 0; margin: 8px;">please revert to the old behavior and check the validity of the returned path in the conditional, then print builtinDir here</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Doh, yes. With my previous implementation the path was never empty when clangBuiltinIncludePath() was called with its default argument. Evidently I had to forget that this was no longer true in the current implementation.</p>
<p style="padding: 0; margin: 8px;">Reverting means we'll be checking the file presence twice in most cases, right? It bothers me a bit to hardcode the filename in 2 locations.</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/D17858#inline-99982">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">clanghelpers.cpp:374</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">the lambda is there to store the result in the <tt style="background: #ebebeb; font-size: 13px;">static const</tt> value that will be returned. we don't want to compute the result multiple times</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Now why does that remind me of the obfuscated C contests of old? :)</p>
<p style="padding: 0; margin: 8px;">I'll add a comment so others won't make the same mistake.</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/D17858#inline-99985">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">clanghelpers.cpp:397</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">actually, thinking about this once more, it can be further simplified similar to the path above:</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);">dir = QStringLiteral(KDEV_CLANG_BUILTIN_DIR "/../../%1/include").arg(clangVersion());
if (QFile::exists(dir + QLatin1Char('/') + entryToCheck)) {
return dir;
}</pre></div>
<p style="padding: 0; margin: 8px;">then all code paths are basically the same in regard to the <tt style="background: #ebebeb; font-size: 13px;">entryToCheck</tt> check, just for different base paths. This could thus be shared, too:</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);">auto isValidDir = [](const QString &dir) {
return QFile::exists(dir + QLatin1String("/cpuid.h"));
}</pre></div>
<p style="padding: 0; margin: 8px;">then use this in the three conditionals</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done. I just added a <tt style="background: #ebebeb; font-size: 13px;">cleanPath()</tt> as in the WIN32 code path, so we don't return a path containing a "detour" (../..).</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/D17858#inline-100009">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">clanghelpers.h:108</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">the code isn't actually only checking for minor upgrades, or?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No, the code just tries to generate the path corresponding to the current version from what it has. In practice this will correspond to minor upgrades because the typical path has 2 version components and we only correct one:</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);">/usr/lib/llvm-6.0/lib/clang/6.0.1/include</pre></div>
<p style="padding: 0; margin: 8px;">We could correct the first one too but it seems very unlikely that the code will ever get to be executed after major upgrades because libclang itself will no longer be in /usr/lib/llvm-6.0/lib (but under /usr/lib/llvm-X.y or, theoretically, /usr/lib/llvm-6.y).</p>
<p style="padding: 0; margin: 8px;">But yeah, if someone decides to install LLVM to a location that does NOT contain the major version number then my code should handle any upgrade or even downgrade, as long as the plugin still loads against the new libclang.</p></div></div></div></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/D17858">https://phabricator.kde.org/D17858</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, flherne, mwolff<br /><strong>Cc: </strong>arrowd, mwolff, flherne, kdevelop-devel, glebaccon, hase, antismap, iodelay, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>