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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 29 05:54:24 PDT 2018


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





More information about the cfe-commits mailing list