dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.
I don't think this should be reused. If I'm reading the code correctly, this is
just a super heavyweight (but cached) call to `Driver::GetResourcesPath()`. It
should probably be deleted instead.
I suggest instead:
1. Use `Driver::GetResourcesPath()` directly in the libclang application.
2. Figure out if it's safe to do that here as well, and if so, delete this and
use it here.
Regarding "is it safe?", the question is whether we expect the
`clang-scan-deps` to be built from the same sources as the `clang` it's
scanning for. I think we do. We're relying on that already for the results of
the scan to be correct.
================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:422-423
if (!HasResourceDir) {
StringRef ResourceDir =
ResourceDirCache.findResourceDir(Args, ClangCLMode);
if (!ResourceDir.empty()) {
----------------
This is likely much faster, since it avoids a lock:
```
lang=c++
assert(!Args.empty());
std::string ResourceDir = Driver::GetResourcePath(Args[0]);
```
But if the string operations turn out to be expensive we could add caching on
top.
Unless, is it supported/expected for `clang-scan-deps` to be from a different
toolchain than `clang`?
================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:424-427
if (!ResourceDir.empty()) {
AdjustedArgs.push_back("-resource-dir");
AdjustedArgs.push_back(std::string(ResourceDir));
}
----------------
I assume this is for performance (to avoid doing the filesystem search in each
invocation)? If so, please add a comment explaining that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108215/new/
https://reviews.llvm.org/D108215
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
