kadircet accepted this revision. kadircet added a subscriber: sammccall. kadircet added a comment. This revision is now accepted and ready to land.
thanks, lgtm! but I would wait for a while to see if someone(that remembers why this test was specifically asserting for built-in headers) will object. ================ Comment at: clang-tools-extra/clangd/test/document-link.test:1 -# for %resource_dir: REQUIRES: clang -# %resource_dir actually points at builtin_include_dir, go up one directory. -# RUN: clangd -lit-test -resource-dir=%resource_dir/.. < %s | FileCheck -strict-whitespace %s + +# create a fake resource_dir so that the test can find the headers. ---------------- nit: drop empty line ================ Comment at: clang-tools-extra/clangd/test/document-link.test:2 + +# create a fake resource_dir so that the test can find the headers. +# RUN: mkdir -p %t/include/ && touch %t/include/foo.h ---------------- s/create/Create/ ================ Comment at: clang-tools-extra/clangd/test/document-link.test:3 +# create a fake resource_dir so that the test can find the headers. +# RUN: mkdir -p %t/include/ && touch %t/include/foo.h +# RUN: clangd -lit-test -resource-dir=%t < %s | FileCheck -strict-whitespace %s ---------------- this is clever! I believe there was a reason for this test to be asserting built-in headers (I can't seem to remember, maybe @sammccall does), but asserting through a mock header should also be fine, I suppose. one thing though, we need to clean up `%t`, e.g. `rm -rf %t`. (and regrading the failure at head, resource_dir path has changed with release cut from `something/12.0.0` to `something/13.0.0` so it should go away once you build new clang via `ninja clang`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95670/new/ https://reviews.llvm.org/D95670 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
