[PATCH] D63986: [clangd] Also cache failures while indexing

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Change looks sensible, just wondering if we should keep the main-file symbols/refs, and whether we can simplify/unify a bit.



================
Comment at: clang-tools-extra/clangd/index/Background.cpp:359
   }
 
   // Build and store new slabs for each updated file.
----------------
couldn't we have a much more unified handling of error/non-error case, by simply dropping all non-main-file shards at this point? (in case of error)

In theory we're processing them a bit, but it practice it seems like they're empty.


================
Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:161
         CI.getDiagnostics().hasUncompilableErrorOccurred()) {
       llvm::errs() << "Skipping TU due to uncompilable errors\n";
+    } else {
----------------
I'm pretty sure the include graph is basically good in this case (up to the possibly-missing files). So we should at least send that, so invalidation works (or can work in the future).

Also, we agreed not to overwrite the symbols for the included headers' shards, but what about the main file itself? Surely better to have something than nothing. (The "something" is roughly clang's recovery, which is what AST-based completions rely on anyway)

More generally, do we need filtering logic at both ends?
It seems like this file could just pass through the data it has, along with the info on whether there was an error (somehow)


================
Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:163
+    } else {
+      // Return empty information for non-compilable TUs.
+      Syms = Collector->takeSymbols();
----------------
comment belongs in the if, not the else


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63986