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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 10 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:156
+  llvm::SmallString<256> Resolved;
+  if (llvm::sys::fs::real_path(Driver, Resolved))
+    return Driver.str();
----------------
kadircet wrote:
> what about taking a VFS instead and calling `VFS.getRealPath`?
> 
> It should make testing easier and commandmangler vfs friendly.
That's a more invasive change I don't feel up to at the moment.
CommandMangler doesn't make much sense if you don't make assumptions about the FS being real (e.g. looking up things on the PATH, using the output of xcrun...)
I don't think testing alone is a strong reason to VFSify it.


================
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.
----------------
kadircet wrote:
> 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) ?
Not a strong reason either way I think.


================
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"));
----------------
kadircet wrote:
> can we rather move the `ifdef` here the rest previous part should hopefully work on other platforms as well (at least after VFS)?
I don't think they'll work: create_link doesn't create a symlink on windows, PATHEXT is a thing, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75414