[PATCH] D146941: [clangd] Use all inputs to SystemIncludeExtractor in cache key

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:80
+  std::string Driver;
+  std::string Directory;
+  // Whether certain includes should be part of query.
----------------
It looks like the purpose of including `Directory` in the key is to use it in `make_absolute` in case where `Driver` is not an absolute path.

I wonder if it would make sense to perform that `make_absolute` operation eagerly and store its result in the key instead.

I'm thinking of a scenario where a project has say 10,000 source files in 500 directories, and in the `compile_commands.json` entries the `directory` is the directory immediately containing the source file (or a corresponding directory in a `build/` directory hierarchy). In such cases, having the directory be in the key would mean executing the driver 500 times instead of just once (assuming no other properties are different) during indexing.


================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:138
+      if (Type == driver::types::TY_INVALID) {
+        elog("System include extraction: invalid file type for {0}", Ext);
+      } else {
----------------
The previous behaviour was to abort system include extraction in this case.

Assuming you're not looking to change that behaviour, one way to preserve it could be to have `render()` check for an empty `Lang` and return an empty vector in that case, and also have `extractSystemIncludesAndTarget()` check for `render()` returning an empty vector and return an empty optional in that case.


================
Comment at: clang-tools-extra/clangd/test/system-include-extractor.test:45
 # can match output lines like "ASTWorker building file"
-# RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test
+# RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test -dump-input=fail
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
----------------
(did you mean to add `-dump-input=fail` here? according to FileCheck docs that's the default)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146941/new/

https://reviews.llvm.org/D146941