dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed.
In D108366#2959736 <https://reviews.llvm.org/D108366#2959736>, @jansvoboda11 wrote: > For `clang-scan-deps`, this is //almost// NFC. This code kicks in iff > `ResourceDirectoryCache` doesn't provide any result: > > - the execution of `CommandLine[0] -print-resource-dir` command returns a > non-zero exit code (exercised in the test) or doesn't print anything, > - the `CommandLine` is empty, > - the compiler executable (`CommandLine[0]`) is not an absolute path. > > That's why I want to check with people if we can agree on removing > `ResourceDirectoryCache`. I'd be keen on updating this patch to include the > change and make the test more obviously correct. Got it. I wonder if it'd be better to split "changing default behaviour of clang-scan-deps the tool" from "avoid inject-resource-dir logic when it's incorrect". I.e., commit something in this patch to fix the immediate problem and remove/optimize ResourceDirectoryCache in a follow-up. For this patch, you could add an inject-resource-dir flag to the scanning service, which controls the new Clang::Tooling flag (and can be controlled by libclang), and a command-line flag in clang-scan-deps to allow testing both sides of it. Would be worth documenting the testcase with a good comment to explain the corner case. Maybe the command-line flag would also disable ResourceDirectoryCache. ================ Comment at: clang/test/ClangScanDeps/resource_directory.c:3 +// RUN: cp %S/Inputs/resource_directory/* %t +// RUN: sed -e "s|CLANG|/our/custom/bin/clang|g" -e "s|DIR|%/t|g" %S/Inputs/resource_directory/cdb_tu.json > %t/cdb.json + ---------------- I don't love this hardcoded path to clang which //could// exist on some machine. Could you use `/dev/null` for the path to clang? Or `/dev/null/bin/clang`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108366/new/ https://reviews.llvm.org/D108366 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
