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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the cleanups, mostly NITs from my side.



================
Comment at: clangd/ClangdServer.cpp:81
 
-  SymbolIndex &index() const { return *MergedIndex; }
+  SymbolIndex &index() { return *MergedIndex; }
 
----------------
Maybe return `const SymbolIndex&` and keep the method const?


================
Comment at: clangd/ClangdServer.cpp:101
+    };
+    return llvm::make_unique<CB>(CB{this});
+  };
----------------
Maybe simplify to `make_unique<CB>(this)`? This should work, right?


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


================
Comment at: clangd/ClangdServer.cpp:152
       WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-                    DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+                    DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
                     Opts.UpdateDebounce, Opts.RetentionPolicy) {
----------------
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.


================
Comment at: clangd/ClangdServer.h:195
+  /// This can be useful for testing, debugging, or observing memory usage.
+  const SymbolIndex *dynamicIndex();
+
----------------
Maybe make it const?


================
Comment at: clangd/TUScheduler.cpp:632
                          bool StorePreamblesInMemory,
-                         ParsingCallbacks &Callbacks,
+                         std::unique_ptr<ParsingCallbacks> Callbacks,
                          std::chrono::steady_clock::duration UpdateDebounce,
----------------
Why not `ParsingCallbacks*` or `ParsingCallbacks&`? This restricts the lifetime of the passed values.
Our particular use-case LG this way, but it seems there is no reason why `TUScheduler` should own the callbacks.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221