ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land.
LGTM ================ 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; ---------------- sammccall wrote: > 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... I would assume the number of symbols in the main files is not very large, compared to the ones from the preamble. We could try indexing those and set a flag they're not for code completion. Should give us a better workspaceSymbol without hurting anything else. ================ Comment at: clangd/ClangdServer.cpp:152 WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory, - DynamicIdx ? *DynamicIdx : noopParsingCallbacks(), + DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr, Opts.UpdateDebounce, Opts.RetentionPolicy) { ---------------- sammccall wrote: > 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). > 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. OTOH, this gives flexibility to the callers wrt to the lifetime, i.e. they don't **have** to create a separate object for the callbacks if they don't want to (one example of this pattern is ClangdLSPServer inheriting DiagnosticsConsumer and ProtocolCallbacks). But happy to do it either way. ================ Comment at: clangd/TUScheduler.h:158 const std::shared_ptr<PCHContainerOperations> PCHOps; - ParsingCallbacks &Callbacks; + std::unique_ptr<ParsingCallbacks> Callbacks; Semaphore Barrier; ---------------- NIT: maybe mention that this is never null in a comment? ================ Comment at: unittests/clangd/TUSchedulerTests.cpp:48 TUScheduler S(getDefaultAsyncThreadsCount(), - /*StorePreamblesInMemory=*/true, noopParsingCallbacks(), + /*StorePreamblesInMemory=*/true, nullptr, /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), ---------------- NIT: maybe add a name of the parameter here for better readability (nullptr can mean many things)? 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
