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