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

[sammccall]

Summary

• 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

[sammccall]

Add note about the overlap between preamble index across files.

[ilya-biryukov]

Thanks for the cleanups, mostly NITs from my side.

clangd/ClangdServer.cpp

81

Maybe return const SymbolIndex& and keep the method const?

101

Maybe simplify to make_unique<CB>(this)? This should work, right?

122

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?

152

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.

clangd/ClangdServer.h

195

Maybe make it const?

clangd/TUScheduler.cpp

632

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.

[sammccall]

clangd/ClangdServer.cpp

122

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...

152

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).

[sammccall]

Address review comments, add missing dynamicIndex() implementation.

clangd/TUScheduler.cpp

632

As noted above, this is just like std::function.

[sammccall]

(forgot some changes)

[ilya-biryukov]

LGTM

clangd/ClangdServer.cpp

122

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.

152

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.

clangd/TUScheduler.h

158

NIT: maybe mention that this is never null in a comment?

unittests/clangd/TUSchedulerTests.cpp

48

NIT: maybe add a name of the parameter here for better readability (nullptr can mean many things)?

[sammccall]

clangd/ClangdServer.cpp

152

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...

[sammccall]

Address review comments

[ilya-biryukov]

clangd/ClangdServer.cpp

152

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).

clangd/ClangdServer.h

public:
   /// The order of results is unspecified.
   /// Overall memory usage of clangd may be significantly more than reported
   /// here, as this metric does not account (at least) for:
   ///   - memory occupied by static and dynamic index,
   ///   - memory required for in-flight requests,
   /// FIXME: those metrics might be useful too, we should add them.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
 
+  /// Returns the active dynamic index if one was built.
+  /// This can be useful for testing, debugging, or observing memory usage.
+  const SymbolIndex *dynamicIndex();
+
   // Blocks the main thread until the server is idle. Only for use in tests.
   // Returns false if the timeout expires.
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds = 10);
 
 private:
   /// FIXME: This stats several files to find a .clang-format file. I/O can be
   /// slow. Think of a way to cache this.
private:
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)
   SymbolIndex *Index;
-  /// If present, an up-to-date of symbols in open files. Read via Index.
+  // If present, an index of symbols in open files. Read via *Index.
   std::unique_ptr<DynamicIndex> DynamicIdx;
-  // If present, a merged view of DynamicIdx and an external index. Read via
-  // Index.
+  // If present, storage for the merged static/dynamic index. Read via *Index.
   std::unique_ptr<SymbolIndex> MergedIndex;
 
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;
   mutable std::mutex CachedCompletionFuzzyFindRequestMutex;
 
   // If set, this represents the workspace path.

clangd/ClangdServer.cpp

void handle(tooling::AtomicChanges SourceReplacements) override {
     assert(!Result.hasValue());
     Result = std::move(SourceReplacements);
   }
 
   Optional<Expected<tooling::AtomicChanges>> Result;
 };
 } // 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<std::string> URISchemes)
       : PreambleIdx(URISchemes), MainFileIdx(URISchemes),
         MergedIndex(mergeIndex(&MainFileIdx, &PreambleIdx)) {}
 
-  SymbolIndex &index() const { return *MergedIndex; }
+  SymbolIndex &index() { return *MergedIndex; }
+
+  // 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<ParsingCallbacks> makeUpdateCallbacks() {
+    struct CB : public ParsingCallbacks {
+      CB(ClangdServer::DynamicIndex *This) : This(This) {}
+      DynamicIndex *This;
 
       void onPreambleAST(PathRef Path, ASTContext &Ctx,
                          std::shared_ptr<clang::Preprocessor> PP) override {
-    PreambleIdx.update(Path, &Ctx, PP, /*TopLevelDecls=*/llvm::None);
+        This->PreambleIdx.update(Path, &Ctx, std::move(PP));
       }
 
       void onMainAST(PathRef Path, ParsedAST &AST) override {
-
-    MainFileIdx.update(Path, &AST.getASTContext(), AST.getPreprocessorPtr(),
+        This->MainFileIdx.update(Path, &AST.getASTContext(),
+                                 AST.getPreprocessorPtr(),
                                  AST.getLocalTopLevelDecls());
       }
+    };
+    return llvm::make_unique<CB>(CB{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<SymbolIndex> MergedIndex;
 };
 
 ClangdServer::Options ClangdServer::optsForTest() {
   ClangdServer::Options Opts;
   Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
   Opts.StorePreamblesInMemory = true;
   Opts.AsyncThreadsCount = 4; // Consistent!
: CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
                      : nullptr),
       PCHs(std::make_shared<PCHContainerOperations>()),
       // Pass a callback into `WorkScheduler` to extract symbols from a newly
       // parsed file and rebuild the file index synchronously each time an AST
       // is parsed.
       // 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(&DynamicIdx->index(), Opts.StaticIndex);
     Index = MergedIndex.get();
   } else if (DynamicIdx)
     Index = &DynamicIdx->index();
   else if (Opts.StaticIndex)
     Index = Opts.StaticIndex;

clangd/ClangdUnit.h

private:
   // Data, stored after parsing.
   std::vector<Diag> Diags;
   // Top-level decls inside the current file. Not that this does not include
   // top-level decls from the preamble.
   std::vector<Decl *> LocalTopLevelDecls;
   IncludeStructure Includes;
 };
 
-using PreambleParsedCallback = std::function<void(
-    PathRef Path, ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
+using PreambleParsedCallback =
+    std::function<void(ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
 
 /// Builds compiler invocation that could be used to build AST or preamble.
 std::unique_ptr<CompilerInvocation>
 buildCompilerInvocation(const ParseInputs &Inputs);
 
 /// Rebuild the preamble for the new inputs unless the old one can be reused.
 /// If \p OldPreamble can be reused, it is returned unchanged.
 /// If \p OldPreamble is null, always builds the preamble.

clangd/ClangdUnit.cpp

CppFilePreambleCallbacks(PathRef File, PreambleParsedCallback ParsedCallback)
       : File(File), ParsedCallback(ParsedCallback) {}
 
   IncludeStructure takeIncludes() { return std::move(Includes); }
 
   void AfterExecute(CompilerInstance &CI) override {
     if (!ParsedCallback)
       return;
     trace::Span Tracer("Running PreambleCallback");
-    ParsedCallback(File, CI.getASTContext(), CI.getPreprocessorPtr());
+    ParsedCallback(CI.getASTContext(), CI.getPreprocessorPtr());
   }
 
   void BeforeExecute(CompilerInstance &CI) override {
     SourceMgr = &CI.getSourceManager();
   }
 
   std::unique_ptr<PPCallbacks> createPPCallbacks() override {
     assert(SourceMgr && "SourceMgr must be set at this point");

clangd/TUScheduler.h

public:
   /// some nodes from the preamble may have been deserialized and may also be
   /// accessed from the main file AST, e.g. redecls of functions from preamble,
   /// etc. Clients are expected to process only the AST nodes from the main file
   /// in this callback (obtained via ParsedAST::getLocalTopLevelDecls) to obtain
   /// optimal performance.
   virtual void onMainAST(PathRef Path, ParsedAST &AST) {}
 };
 
-ParsingCallbacks &noopParsingCallbacks();
-
 /// Handles running tasks for ClangdServer and managing the resources (e.g.,
 /// preambles and ASTs) for opened files.
 /// TUScheduler is not thread-safe, only one thread should be providing updates
 /// and scheduling tasks.
 /// Callbacks are run on a threadpool and it's appropriate to do slow work in
 /// them. Each task has a name, used for tracing (should be UpperCamelCase).
 /// FIXME(sammccall): pull out a scheduler options struct.
 class TUScheduler {
 public:
   TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory,
-              ParsingCallbacks &ASTCallbacks,
+              std::unique_ptr<ParsingCallbacks> ASTCallbacks,
               std::chrono::steady_clock::duration UpdateDebounce,
               ASTRetentionPolicy RetentionPolicy);
   ~TUScheduler();
 
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
 
public:
   // this inside clangd.
   // FIXME: remove this when there is proper index support via build system
   // integration.
   static llvm::Optional<llvm::StringRef> getFileBeingProcessedInContext();
 
 private:
   const bool StorePreamblesInMemory;
   const std::shared_ptr<PCHContainerOperations> PCHOps;
-  ParsingCallbacks &Callbacks;
+  std::unique_ptr<ParsingCallbacks> Callbacks;
   Semaphore Barrier;
   llvm::StringMap<std::unique_ptr<FileData>> Files;
   std::unique_ptr<ASTCache> IdleASTs;
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   llvm::Optional<AsyncTaskRunner> PreambleTasks;
   llvm::Optional<AsyncTaskRunner> WorkerThreads;
   std::chrono::steady_clock::duration UpdateDebounce;

clangd/TUScheduler.cpp

if (!Invocation) {
       // Make sure anyone waiting for the preamble gets notified it could not
       // be built.
       PreambleWasBuilt.notify();
       return;
     }
 
     std::shared_ptr<const PreambleData> OldPreamble =
         getPossiblyStalePreamble();
-    std::shared_ptr<const PreambleData> NewPreamble =
-        buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
-                      PCHs, StorePreambleInMemory,
-                      [this](PathRef Path, ASTContext &Ctx,
-                             std::shared_ptr<clang::Preprocessor> PP) {
-                        Callbacks.onPreambleAST(FileName, Ctx, std::move(PP));
-                      });
+    std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
+        FileName, *Invocation, OldPreamble, OldCommand, Inputs, PCHs,
+        StorePreambleInMemory,
+        [this](ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP) {
 
     bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
       LastBuiltPreamble = NewPreamble;
     }
     // Before doing the expensive AST reparse, we want to release our reference
     // to the old preamble, so it can be freed if there are no other references
unsigned getDefaultAsyncThreadsCount() {
   // C++ standard says that hardware_concurrency()
   // may return 0, fallback to 1 worker thread in
   // that case.
   if (HardwareConcurrency == 0)
     return 1;
   return HardwareConcurrency;
 }
 
-ParsingCallbacks &noopParsingCallbacks() {
-  static ParsingCallbacks *Instance = new ParsingCallbacks;
-  return *Instance;
-}
-
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
   std::string Contents;
   tooling::CompileCommand Command;
   ASTWorkerHandle Worker;
 };
 
 TUScheduler::TUScheduler(unsigned AsyncThreadsCount,
                          bool StorePreamblesInMemory,
-                         ParsingCallbacks &Callbacks,
+                         std::unique_ptr<ParsingCallbacks> Callbacks,
                          std::chrono::steady_clock::duration UpdateDebounce,
                          ASTRetentionPolicy RetentionPolicy)
     : StorePreamblesInMemory(StorePreamblesInMemory),
-      PCHOps(std::make_shared<PCHContainerOperations>()), Callbacks(Callbacks),
+      PCHOps(std::make_shared<PCHContainerOperations>()),
+      Callbacks(Callbacks ? move(Callbacks)
+                          : llvm::make_unique<ParsingCallbacks>()),
       Barrier(AsyncThreadsCount),
       IdleASTs(llvm::make_unique<ASTCache>(RetentionPolicy.MaxRetainedASTs)),
       UpdateDebounce(UpdateDebounce) {
   if (0 < AsyncThreadsCount) {
     PreambleTasks.emplace();
     WorkerThreads.emplace();
   }
 }

 void TUScheduler::update(
     PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags,
     llvm::unique_function<void(std::vector<Diag>)> OnUpdated) {
   std::unique_ptr<FileData> &FD = Files[File];
   if (!FD) {
     // Create a new worker to process the AST-related tasks.
     ASTWorkerHandle Worker = ASTWorker::create(
         File, *IdleASTs, WorkerThreads ? WorkerThreads.getPointer() : nullptr,
-        Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory, Callbacks);
+        Barrier, UpdateDebounce, PCHOps, StorePreamblesInMemory, *Callbacks);
     FD = std::unique_ptr<FileData>(new FileData{
         Inputs.Contents, Inputs.CompileCommand, std::move(Worker)});
   } else {
     FD->Contents = Inputs.Contents;
     FD->Command = Inputs.CompileCommand;
   }
   FD->Worker->update(std::move(Inputs), WantDiags, std::move(OnUpdated));
 }

unittests/clangd/FileIndexTests.cpp

TEST(FileIndexTest, RebuildWithPreamble) {
   // Rebuild the file.
   auto CI = buildCompilerInvocation(PI);
 
   FileIndex Index;
   bool IndexUpdated = false;
   buildPreamble(
       FooCpp, *CI, /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
       std::make_shared<PCHContainerOperations>(), /*StoreInMemory=*/true,
-      [&Index, &IndexUpdated](PathRef FilePath, ASTContext &Ctx,
-                              std::shared_ptr<Preprocessor> PP) {
+      [&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP) {
         EXPECT_FALSE(IndexUpdated) << "Expected only a single index update";
         IndexUpdated = true;
-        Index.update(FilePath, &Ctx, std::move(PP));
+        Index.update(FooCpp, &Ctx, std::move(PP));
       });
   ASSERT_TRUE(IndexUpdated);
 
   // Check the index contains symbols from the preamble, but not from the main
   // file.
   FuzzyFindRequest Req;
   Req.Query = "";
   Req.Scopes = {"", "ns_in_header::"};

unittests/clangd/TUSchedulerTests.cpp

protected:
 
   llvm::StringMap<std::string> Files;
   llvm::StringMap<time_t> Timestamps;
   MockCompilationDatabase CDB;
 };
 
 TEST_F(TUSchedulerTests, MissingFiles) {
   TUScheduler S(getDefaultAsyncThreadsCount(),
-                /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+                /*StorePreamblesInMemory=*/true, nullptr,
                 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
                 ASTRetentionPolicy());
 
   auto Added = testPath("added.cpp");
   Files[Added] = "";
 
   auto Missing = testPath("missing.cpp");
   Files[Missing] = "";

 TEST_F(TUSchedulerTests, WantDiagnostics) {
   std::atomic<int> CallbackCount(0);
   {
     // To avoid a racy test, don't allow tasks to actualy run on the worker
     // thread until we've scheduled them all.
     Notification Ready;
     TUScheduler S(
         getDefaultAsyncThreadsCount(),
-        /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+        /*StorePreamblesInMemory=*/true, nullptr,
         /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
         ASTRetentionPolicy());
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
              [&](std::vector<Diag>) { Ready.wait(); });
 
     S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
              [&](std::vector<Diag> Diags) { ++CallbackCount; });
TEST_F(TUSchedulerTests, WantDiagnostics) {
   }
   EXPECT_EQ(2, CallbackCount);
 }
 
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic<int> 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.
     auto Path = testPath("foo.cpp");
     S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
              [&](std::vector<Diag> Diags) {
                ADD_FAILURE() << "auto should have been debounced and canceled";
              });
TEST_F(TUSchedulerTests, ManyUpdates) {
   std::mutex Mut;
   int TotalASTReads = 0;
   int TotalPreambleReads = 0;
   int TotalUpdates = 0;
 
   // Run TUScheduler and collect some stats.
   {
     TUScheduler S(getDefaultAsyncThreadsCount(),
-                  /*StorePreamblesInMemory=*/true, noopParsingCallbacks(),
+                  /*StorePreamblesInMemory=*/true, nullptr,
                   /*UpdateDebounce=*/std::chrono::milliseconds(50),
                   ASTRetentionPolicy());
 
     std::vector<std::string> Files;
     for (int I = 0; I < FilesCount; ++I) {
       std::string Name = "foo" + std::to_string(I) + ".cpp";
       Files.push_back(testPath(Name));
       this->Files[Files.back()] = "";
TEST_F(TUSchedulerTests, ManyUpdates) {
   EXPECT_EQ(TotalPreambleReads, FilesCount * UpdatesPerFile);
 }
 
 TEST_F(TUSchedulerTests, EvictedAST) {
   std::atomic<int> BuiltASTCounter(0);
   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(
     int* a;
     double* b = a;
   )cpp";
   llvm::StringLiteral OtherSourceContents = R"cpp(
     int* a;
TEST_F(TUSchedulerTests, EvictedAST) {
   // Check the AST for foo.cpp is retained now and one of the others got
   // evicted.
   EXPECT_THAT(S.getFilesWithCachedAST(),
               UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
 }
 
 TEST_F(TUSchedulerTests, EmptyPreamble) {
   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");
   auto Header = testPath("foo.h");
 
   Files[Header] = "void foo()";
   Timestamps[Header] = time_t(0);
S.runWithPreamble("getEmptyPreamble", Foo,
                                 0u);
                     });
 }
 
 TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   // 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");
   auto NonEmptyPreamble = R"cpp(
     #define FOO 1
     #define BAR 2
 
     int main() {}
TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
   std::lock_guard<std::mutex> Lock(PreamblesMut);
   ASSERT_NE(Preambles[0], nullptr);
   ASSERT_THAT(Preambles, Each(Preambles[0]));
 }
 
 TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   TUScheduler S(
       /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-      /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+      /*StorePreambleInMemory=*/true, nullptr,
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
 
   auto Source = testPath("foo.cpp");
   auto Header = testPath("foo.h");
 
   Files[Header] = "int a;";
   Timestamps[Header] = time_t(0);
TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
   ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents)));
   ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
 }
 
 TEST_F(TUSchedulerTests, NoChangeDiags) {
   TUScheduler S(
       /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
-      /*StorePreambleInMemory=*/true, noopParsingCallbacks(),
+      /*StorePreambleInMemory=*/true, nullptr,
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
       ASTRetentionPolicy());
 
   auto FooCpp = testPath("foo.cpp");
   auto Contents = "int a; int b;";
 
   S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No,
            [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });