[PATCH] D47063: [clangd] Keep only a limited number of idle ASTs in memory

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Impl looks good.
Is there a way we can reasonably test this? (specifically that we don't just store all the ASTs forever)



================
Comment at: clangd/ClangdUnit.h:143
+std::shared_ptr<const PreambleData>
+buildPreamble(PathRef FileName, CompilerInvocation &CI,
+              std::shared_ptr<const PreambleData> OldPreamble,
----------------
nit: i think filename here is only used for logging, just use Inputs.CompileCommand.Filename?


================
Comment at: clangd/TUScheduler.cpp:103
+
+  /// Returns the cached value for \p K, or llvm::None if the value is not in
+  /// the cache anymore.
----------------
please also explicitly mention what nullptr means (even though that's really up to the caller of put()). None vs nullptr leaves room for confusion.


================
Comment at: clangd/TUScheduler.h:66
+              std::chrono::steady_clock::duration UpdateDebounce,
+              ASTRetentionPolicy RetentionPolicy = {});
   ~TUScheduler();
----------------
does this actually have more than one caller? what's the plan for exposing this option to embedders/CLI users (not saying we necessarily need the latter)?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47063