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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 3 inline comments as done.
kadircet 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.
----------------
nridge wrote:
> 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.
yeah makes sense. i was mostly trying to avoid the extra string operations, with the assumption of "directory" being ~always the same across entries. but we're already paying for those string operations today, so not saving them won't be a regression and being more resilient for such cases in the future wouldn't hurt.


================
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 {
----------------
nridge wrote:
> 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.
thanks! definitely didn't notice that, I am not sure how valid of a concern this is in practice. as we'll only use the cached result for translation units whose language cannot be determined (I'd claim they all belong to same category). but leaving it as-is in this patch with a fixme (happy to drop the fixme if you think that's actually important). made it similar to existing code pattern, i.e. bailing out early if language is empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146941