sammccall marked 3 inline comments as done. sammccall added inline comments.
================ Comment at: clangd/ClangdServer.cpp:122 + // - symbols declared both in the main file and the preamble + // (Note that symbols *only* in the main file are not indexed). FileIndex MainFileIdx; ---------------- ilya-biryukov wrote: > I thought it was not true, but now I notice we actually don't index those > symbols. > I think we should index them for workspaceSymbols, but not for completion. > Any pointers to the code that filters them out? https://reviews.llvm.org/source/clang-tools-extra/browse/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp;340828$272 isExpansionInMainFile() (having this buried so deep hurts readability and probably performance). But I think this would be the behavior we want for code complete, to keep the indexes tiny and incremental updates fast? WorkspaceSymbols is indeed a problem here :-( Tradeoffs... ================ Comment at: clangd/ClangdServer.cpp:152 WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory, - DynamicIdx ? *DynamicIdx : noopParsingCallbacks(), + DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr, Opts.UpdateDebounce, Opts.RetentionPolicy) { ---------------- ilya-biryukov wrote: > Any reason to prefer `nullptr` over the no-op callbacks? > Might be a personal preference, my reasoning is: having an extra check for > null before on each of the call sites both adds unnecessary boilerplate and > adds an extra branch before the virtual call (which is, technically, another > form of a branch). > > Not opposed to doing it either way, though. Basically I prefer the old behavior here (when it was std::function). Having to keep the callbacks object alive is a pain, and the shared instance of the no-op implementation for this purpose seems a little silly. We don't have the std::function copyability stuff which is a mixed bag but nice for cases like this. But passing the reference from TUScheduler to its ASTWorkers is internal enough that it doesn't bother me, WDYT? > extra check for null before on each of the call sites Note that the check is actually in the constructor, supporting `nullptr` is just for brevity (particularly in tests). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51221 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
