[PATCH] D108215: [clang][deps] NFC: Move `ResourceDirectoryCache`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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