Bigcheese requested changes to this revision. Bigcheese added a comment. This revision now requires changes to proceed.
So I agree solving this problem makes sense, but I have some issues with the current patch. - This defaults to doing the lookup. The test suite, and some real outputs, just uses `"clang ..."` as the command, not an absolute path. If we always do the lookup then this would be whatever `clang` is in `PATH`, not the `clang` we're testing. - While looking into the above, I think I discovered that the way `FindResourceDir ` constructs `ClangBinaryPath` is broken. It uses the `"directory"` entry from the compilation database, which is the working directory of where the compile occurred. It should be extremely rare for this ever to contain a `clang`, and even rarer for it to be the one that was intended. - This touches a lot of `clang/lib/Tooling` that I don't think it needs to. For the first issue we may just want to fix the tests to point to a specific `clang`, it's annoying, but doable. An alternative would be to //only// do `FindResourceDir` when given an absolute path to a compiler, instead of trying to guess. I think I prefer this approach, as if you have an installed `clang` in your path, there should be a `clang-scan-deps` next to it, which would use the same resource dir. For the second and third issues, I think you can just remove the `StringRef Directory` arg and use `FindInEnvPath` to get the correct `clang`. You shouldn't need it anymore, but you can replace `MakeAbsolutePath` with `llvm::sys::path::make_absolute`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69122/new/ https://reviews.llvm.org/D69122 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
