sammccall added a comment.
Thanks, this looks like a good idea to me, but some quibbles with the details:
- for any string, we statically know whether it's path or WD-relative
- we should make sure all the IO gets cached
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:334
llvm::SmallString<128> Driver(Cmd->CommandLine.front());
llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
----------------
Oops, this has come up before (in CommandMangler).
Your idea here makes sense (if the driver is "clang" we should look it up on
the PATH) and the approach is backwards-compatible.
However when we think more carefully, by allowing the driver to be both
WD-relative and dir-relative, we're emulating shell behavior (which makes
sense, we have an argv!).
But shells don't generally try one then the other, they instead distinguish
three cases:
- bare name `clang` is looked up on PATH
- relative path `bin/clang` is resolved against WD
- absolute path `/usr/bin/clang` is used as-is
So I think we'd rather have something like:
```
if (llvm::none_of(Driver,
[](char C) { return llvm::sys::path::is_separator(C); })) {
// resolve against path
} else {
make_absolute(Cmd->Directory, Driver);
}
```
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:337
+ if (!llvm::sys::fs::exists(Driver)) {
+ auto DriverProgram =
----------------
I'm not sure it's reasonable to do IO like this before we have a chance to hit
the cache (but see above comment, I don't think we need the `exists()` at all)
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:339
+ auto DriverProgram =
+ llvm::sys::findProgramByName(Cmd->CommandLine.front());
+ if (DriverProgram) {
----------------
hmm, this also does IO, and we obviously do need this.
I think we need to move this into the cached lookup, so:
- if the path is "bin/clangd" we resolve it to absolute before querying the
cache
- if the path is "/usr/bin/clangd" we pass that absolute path to cache
- but if the path is "clangd" we pass that exact string into
extractSystemIncludesAndTarget(), which notices that it's relative and looks it
up on PATH. Fortunately we can assume PATH doesn't change while running, so the
cache is still correct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93600/new/
https://reviews.llvm.org/D93600
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
