[PATCH] D75414: [clangd] Resolve driver symlinks, and look up unknown relative drivers in PATH.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:145
+    // Otherwise try to look it up on PATH. This won't change basename.
+    if (auto Absolute = llvm::sys::findProgramByName(Driver))
+      Driver = Storage = *Absolute;
----------------
`findProgramByName` is evil :/ it is not guaranteed to return an absolute path.

for example if you've got `toolchain/clang` as your argv[0] (not sure if it is possible, but it is a non-absoltue path...) then it will return the argument directly, even though it is not an absolute path :(.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:156
+  llvm::SmallString<256> Resolved;
+  if (llvm::sys::fs::real_path(Driver, Resolved))
+    return Driver.str();
----------------
what about taking a VFS instead and calling `VFS.getRealPath`?

It should make testing easier and commandmangler vfs friendly.


================
Comment at: clang-tools-extra/clangd/CompileCommands.h:48
   CommandMangler() = default;
+  Memoize<llvm::StringMap<std::string>> CachedResolveDriver;
 };
----------------
maybe just `ResolvedDriver`?


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:286
+  // Caches includes extracted from a driver. Key is driver:lang.
+  Memoize<llvm::StringMap<std::vector<std::string>>> DriverToIncludesCache;
+  llvm::Regex QueryDriverRegex;
----------------
nit: `QueriedDrivers`


================
Comment at: clang-tools-extra/clangd/Threading.h:154
+  template <typename T, typename Func>
+  typename Container::mapped_type operator()(T &&Key, Func Compute) const {
+    {
----------------
what about an explicit `getOrCreate` method instead?


================
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:140
+  std::vector<std::string> Cmd = {(TempDir + "/bin/foo").str(), "foo.cc"};
+  Mangler.adjust(Cmd);
+  // Directory based on resolved symlink, basename preserved.
----------------
irrelevant to the patch:

any reason for `adjust` to mutate its parameter in-place instead of returning it(I believe callers can do `std::move(Cmd)` if need be) ?


================
Comment at: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp:144
+
+  // Set PATH to point to temp/bin so we can find 'foo' on it.
+  ASSERT_TRUE(::getenv("PATH"));
----------------
can we rather move the `ifdef` here the rest previous part should hopefully work on other platforms as well (at least after VFS)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75414/new/

https://reviews.llvm.org/D75414