[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good, just a couple of nits.

I guess you don't have commit access, I'm happy to land this for you when you're happy with it. Just need an email address to associate the commit with.

In D93600#2467393 <https://reviews.llvm.org/D93600#2467393>, @rapgenic wrote:

> About the test, I thought it would be reasonable to test the three cases separately, however I cannot get it to work with a single test file (I think it's some problem with the generated files). Should I duplicate it or is there any way to do it better?

Yeah, these tests are a pain. I think you'd have to duplicate it, but honestly that's a maintenance trap, and it's really hard to factor out common logic in lit tests.
I think folding in the complicated case into the existing test as you've done is probably best.



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:145
+
+  if (!llvm::sys::path::is_absolute(Driver)) {
+    auto DriverProgram = llvm::sys::findProgramByName(Driver);
----------------
maybe add an `assert` inside this if that there are no separators?


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:151
+      Driver = *DriverProgram;
+    }
+  }
----------------
else log and return none?
We don't want to continue if we couldn't find `clang` on the path.


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:346
     llvm::SmallString<128> Driver(Cmd->CommandLine.front());
-    llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+    if (!llvm::none_of(Driver,
+                       [](char C) { return llvm::sys::path::is_separator(C); }))
----------------
nit: `!llvm::none_of` -> `llvm::any_of`


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

https://reviews.llvm.org/D93600