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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
