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