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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
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:
> 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.
> 
I don't think there's any flexibility here, callers *can't* in general create an object (unless they know enough about the lifetimes to store the object appropriately).

For dependencies of the callback (e.g. lambda ref captures), either way works well.
For resources owned by the callback itself, the callee knows better than the caller when they should be freed.

> ClangdLSPServer inheriting DiagnosticsConsumer and ProtocolCallbacks
FWIW, the former seems like an antipattern that should be std::function, to me.
The latter is indeed different - interfaces with many methods no longer feel like they fit the lightweight callback pattern, to me...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221