D17858: clang: Also detect Clang builtin dirs at runtime on Unix

[René J.V. Bertin]

rjvbb marked 3 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> mwolff wrote in clangsupport.cpp:188
> the path is empty, so nothing will be printed here
> 
> please revert to the old behavior and check the validity of the returned path in the conditional, then print builtinDir here

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.

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.

> mwolff wrote in clanghelpers.cpp:374
> the lambda is there to store the result in the `static const` value that will be returned. we don't want to compute the result multiple times

Now why does that remind me of the obfuscated C contests of old? :)

I'll add a comment so others won't make the same mistake.

> mwolff wrote in clanghelpers.cpp:397
> actually, thinking about this once more, it can be further simplified similar to the path above:
> 
>   dir = QStringLiteral(KDEV_CLANG_BUILTIN_DIR "/../../%1/include").arg(clangVersion());
>   if (QFile::exists(dir + QLatin1Char('/') + entryToCheck)) {
>       return dir;
>   }
> 
> then all code paths are basically the same in regard to the `entryToCheck` check, just for different base paths. This could thus be shared, too:
> 
>   auto isValidDir = [](const QString &dir) {
>       return QFile::exists(dir + QLatin1String("/cpuid.h"));
>   }
> 
> then use this in the three conditionals

Done. I just added a `cleanPath()` as in the WIN32 code path, so we don't return a path containing a "detour" (../..).

> mwolff wrote in clanghelpers.h:108
> the code isn't actually only checking for minor upgrades, or?

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:

  /usr/lib/llvm-6.0/lib/clang/6.0.1/include

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).

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.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D17858

To: rjvbb, #kdevelop, flherne, mwolff
Cc: arrowd, mwolff, flherne, kdevelop-devel, glebaccon, hase, antismap, iodelay, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190110/1af81054/attachment-0001.html>