[PATCH] D51221: [clangd] Some nitpicking around the new split (preamble/main) dynamic index

[Sam McCall via Phabricator via cfe-commits]

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