[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 22 14:20:09 PST 2020
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
More information about the cfe-commits
mailing list