[PATCH] D55650: [clangd] Fix an assertion failure in background index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I think the description is misleading, you are saying that we were triggering an assertion and you are fixing that. But in reality, we were not triggering any assertions, this patch is adding the assertions right?



================
Comment at: clangd/index/Background.cpp:385
 
+  assert(Index.Symbols && Index.Refs && Index.Sources
+     && "Symbols, Refs and Sources must be set.");
----------------
I don't think it is a good idea to assert and die just for one file, since we are actually trying to index all the files. We should rather check these exists and propagate the error in case of failure. You can have a look at https://reviews.llvm.org/D55224, same lines.

Also with your changes in the endsourcefileaction this assertion is not possible to be triggered, since you will fill them with "empty" versions.


================
Comment at: clangd/index/IndexAction.cpp:144
+        CI.getDiagnostics().hasUncompilableErrorOccurred();
+    if (!CI.hasDiagnostics() || !HasUncompilableErrors) {
+      Symbols = Collector->takeSymbols();
----------------
I think this is same as `if (!HasUncompilableErrors)` so you can keep the old one just by negating it.


================
Comment at: clangd/index/IndexAction.cpp:150
+    assert(SymbolsCallback && "SymbolsCallback must be set.");
+    SymbolsCallback(std::move(Symbols));
+    if (RefsCallback)
----------------
I don't think it is a good idea to respond with an "empty" symbol slab rather than not replying. They have different semantics. The former implies everything went well, just couldn't find any symbols in the file, whereas the latter implies there was an error.

So I believe we should keep this code as it was.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650