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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D55650#1329692 <https://reviews.llvm.org/D55650#1329692>, @kadircet wrote:

> 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?


Ah, sorry for the confusion caused by the assertion I added. The description is correct, and this patch is trying to fix an assertion (not the assertion I added, it is the assertion in `llvm::Optional`, we are accessing an empty `Optional`).



================
Comment at: clangd/index/Background.cpp:385
 
+  assert(Index.Symbols && Index.Refs && Index.Sources
+     && "Symbols, Refs and Sources must be set.");
----------------
kadircet wrote:
> 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.
`check these exists` seems a wired way to me, We should explicitly check the error here.

The assertion I added here is to guarantee that we have all the data when indexing a file successfully. 


================
Comment at: clangd/index/IndexAction.cpp:150
+    assert(SymbolsCallback && "SymbolsCallback must be set.");
+    SymbolsCallback(std::move(Symbols));
+    if (RefsCallback)
----------------
kadircet wrote:
> 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.
Sounds fair, I kept the old behavior.
but we have different behavior in `clangd-indexer` -- `clangd-indexer` will emit an empty index file when processing a problematic cpp file... a tradeoff is that we will reindex the problematic cpp file since we don't emit any index file, and the `backgroundindex `  thinks this file is not being indexed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55650