[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341325: [clangd] Some nitpicking around the new split 
(preamble/main) dynamic index (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51221?vs=163730=163732#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51221

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.h
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.h
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.h
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -71,34 +71,57 @@
 };
 } // namespace
 
-/// Manages dynamic index for open files. Each file might contribute two sets
-/// of symbols to the dynamic index: symbols from the preamble and symbols
-/// from the file itself. Those have different lifetimes and we merge results
-/// from both
-class ClangdServer::DynamicIndex : public ParsingCallbacks {
+/// The dynamic index tracks symbols visible in open files.
+/// For boring reasons, it doesn't implement SymbolIndex directly - use index().
+class ClangdServer::DynamicIndex {
 public:
   DynamicIndex(std::vector URISchemes)
   : PreambleIdx(URISchemes), MainFileIdx(URISchemes),
 MergedIndex(mergeIndex(, )) {}
 
-  SymbolIndex () const { return *MergedIndex; }
+  const SymbolIndex () const { return *MergedIndex; }
 
-  void onPreambleAST(PathRef Path, ASTContext ,
- std::shared_ptr PP) override {
-PreambleIdx.update(Path, , PP, /*TopLevelDecls=*/llvm::None);
-  }
-
-  void onMainAST(PathRef Path, ParsedAST ) override {
+  // Returns callbacks that can be used to update the index with new ASTs.
+  // Index() presents a merged view of the supplied main-file and preamble ASTs.
+  std::unique_ptr makeUpdateCallbacks() {
+struct CB : public ParsingCallbacks {
+  CB(ClangdServer::DynamicIndex *This) : This(This) {}
+  DynamicIndex *This;
+
+  void onPreambleAST(PathRef Path, ASTContext ,
+ std::shared_ptr PP) override {
+This->PreambleIdx.update(Path, , std::move(PP));
+  }
 
-MainFileIdx.update(Path, (), AST.getPreprocessorPtr(),
-   AST.getLocalTopLevelDecls());
-  }
+  void onMainAST(PathRef Path, ParsedAST ) override {
+This->MainFileIdx.update(Path, (),
+ AST.getPreprocessorPtr(),
+ AST.getLocalTopLevelDecls());
+  }
+};
+return llvm::make_unique(this);
+  };
 
 private:
+  // Contains information from each file's preamble only.
+  // These are large, but update fairly infrequently (preambles are stable).
+  // Missing information:
+  //  - symbol occurrences (these are always "from the main file")
+  //  - definition locations in the main file
+  //
+  // FIXME: Because the preambles for different TUs have large overlap and
+  // FileIndex doesn't deduplicate, this uses lots of extra RAM.
+  // The biggest obstacle in fixing this: the obvious approach of partitioning
+  // by declaring file (rather than main file) fails if headers provide
+  // different symbols based on preprocessor state.
   FileIndex PreambleIdx;
+  // Contains information from each file's main AST.
+  // These are updated frequently (on file change), but are relatively small.
+  // Mostly contains:
+  //  - occurrences of symbols declared in the preamble and referenced from main
+  //  - symbols declared both in the main file and the preamble
+  // (Note that symbols *only* in the main file are not indexed).
   FileIndex MainFileIdx;
-  /// Merged view into both indexes. Merges are performed in a similar manner
-  /// to the merges of dynamic and static index.
   std::unique_ptr MergedIndex;
 };
 
@@ -127,7 +150,7 @@
   // FIXME(ioeric): this can be slow and we may be able to index on less
   // critical paths.
   WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
 Opts.UpdateDebounce, Opts.RetentionPolicy) {
   if (DynamicIdx && Opts.StaticIndex) {
 MergedIndex = mergeIndex(>index(), Opts.StaticIndex);
@@ -144,6 +167,10 @@
 // ClangdServer::DynamicIndex.
 ClangdServer::~ClangdServer() = default;
 
+const SymbolIndex *ClangdServer::dynamicIndex() const {
+  return 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163730.
sammccall marked 2 inline comments as done.
sammccall added a comment.

Address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -258,7 +258,7 @@
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -308,7 +308,7 @@
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -355,7 +355,7 @@
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
   /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +388,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +441,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, /*ASTCallbacks=*/nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [, ](PathRef FilePath, ASTContext ,
-  std::shared_ptr PP) {
+  [&](ASTContext , std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, , std::move(PP));
+Index.update(FooCpp, , std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -74,8 +74,6 @@
   virtual void onMainAST(PathRef Path, ParsedAST ) {}
 };
 
-ParsingCallbacks ();
-
 /// 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 PCHOps;
-  ParsingCallbacks 
+  std::unique_ptr 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163023.
sammccall added a comment.

(forgot some changes)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [, ](PathRef FilePath, ASTContext ,
-  std::shared_ptr PP) {
+  [&](ASTContext , std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, , std::move(PP));
+Index.update(FooCpp, , std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -74,8 +74,6 @@
   

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 163013.
sammccall marked an inline comment as done.
sammccall added a comment.

Address review comments, add missing dynamicIndex() implementation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [, ](PathRef FilePath, ASTContext ,
-  std::shared_ptr PP) {
+  [&](ASTContext , std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, , std::move(PP));
+Index.update(FooCpp, , std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h

[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/TUScheduler.cpp:632
  bool StorePreamblesInMemory,
- ParsingCallbacks ,
+ std::unique_ptr Callbacks,
  std::chrono::steady_clock::duration UpdateDebounce,

ilya-biryukov wrote:
> 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.
As noted above, this is just like std::function.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 () const { return *MergedIndex; }
+  SymbolIndex () { return *MergedIndex; }
 

Maybe return `const SymbolIndex&` and keep the method const?



Comment at: clangd/ClangdServer.cpp:101
+};
+return llvm::make_unique(CB{this});
+  };

Maybe simplify to `make_unique(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 ,
+ std::unique_ptr 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 162401.
sammccall added a comment.

Add note about the overlap between preamble index across files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
   std::make_shared(), /*StoreInMemory=*/true,
-  [, ](PathRef FilePath, ASTContext ,
-  std::shared_ptr PP) {
+  [&](ASTContext , std::shared_ptr PP) {
 EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
 IndexUpdated = true;
-Index.update(FilePath, , std::move(PP));
+Index.update(FooCpp, , std::move(PP));
   });
   ASSERT_TRUE(IndexUpdated);
 
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, javed.absar.

- DynamicIndex doesn't implement ParsingCallbacks, to make its role clearer. 
ParsingCallbacks is a separate object owned by the receiving TUScheduler. (I 
tried to get rid of the "index-like-object that doesn't implement index" but it 
was too messy).
- Clarified(?) docs around DynamicIndex - fewer details up front, more details 
inside.
- Exposed dynamic index from ClangdServer for memory monitoring and more direct 
testing of its contents (actual tests not added here, wanted to get this out 
for review)
- Removed a redundant and sligthly confusing filename param in a callback


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51221

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -45,7 +45,7 @@
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 
@@ -101,7 +101,7 @@
 Notification Ready;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+/*StorePreamblesInMemory=*/true, nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
 ASTRetentionPolicy());
 auto Path = testPath("foo.cpp");
@@ -129,7 +129,7 @@
   std::atomic CallbackCount(0);
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
   ASTRetentionPolicy());
 // FIXME: we could probably use timeouts lower than 1 second here.
@@ -160,7 +160,7 @@
   // Run TUScheduler and collect some stats.
   {
 TUScheduler S(getDefaultAsyncThreadsCount(),
-  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreamblesInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
   ASTRetentionPolicy());
 
@@ -257,8 +257,7 @@
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
   TUScheduler S(
-  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
 
   llvm::StringLiteral SourceContents = R"cpp(
@@ -307,8 +306,7 @@
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -354,8 +352,7 @@
   // Testing strategy: we update the file and schedule a few preamble reads at
   // the same time. All reads should get the same non-null preamble.
   TUScheduler S(
-  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
-  noopParsingCallbacks(),
+  /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
   auto Foo = testPath("foo.cpp");
@@ -388,7 +385,7 @@
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
@@ -441,7 +438,7 @@
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
   /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-  /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+  /*StorePreambleInMemory=*/true, nullptr,
   /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
   ASTRetentionPolicy());
 
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -251,11 +251,10 @@
   buildPreamble(
   FooCpp, *CI,