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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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) {
----------------
sammccall wrote:
> 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...
> 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).
The only limitation is that their object must outlive the ClangdServer, they have flexibility in making it a separate object, a subobject of some other object, a global static object in case it does not have state.
Any option other than separate object requires extra code when `unique_ptr` is accepted.
OTOH, I agree that keeping it alive is a pain, and maybe it's the most common use-case anyway.

But again, not opposed to either of these approaches.

> FWIW, the former seems like an antipattern that should be std::function, to me.
Agreed, I would also prefer for it to be an `std::function` or a separate class that implements the interface. To separate the implementation of different concerns (LSP managing vs handling ClangdServer responses) better.
In that sense, making the callee responsible for the lifetime of the object forces this separation on the callers, which actually looks like a good thing.

> The latter is indeed different - interfaces with many methods no longer feel like they fit the lightweight callback pattern, to me...
It's hard to draw a line between "lightweight callback pattern" and a non-lightweight one, it feels like this shouldn't affect the API choices (the only clear separation I see is between one callback function vs two and more, since we have nice syntax with lambdas for single-callback case).


Repository:
  rL LLVM

https://reviews.llvm.org/D51221