[PATCH] D41991: [clangd] Always use preamble (even stale) for code completion

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Herald added a reviewer: jkorous-apple.

Big +1 to the change here.
Just suggestions for the comments.



================
Comment at: clangd/CodeComplete.cpp:522
 
-  // Attempt to reuse the PCH from precompiled preamble, if it was built.
-  if (Preamble) {
-    auto Bounds =
-        ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
-    if (!Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, VFS.get()))
-      Preamble = nullptr;
-  }
-
+  // Note that we delibirately don't check if preamble is up-to-date. This
+  // operation is very expensive and we feel the right trade-off here is to
----------------
delibirately -> deliberately

Thanks for adding the motivation here! I think this can be a bit terser, but up to you.
e.g.
  // We reuse the preamble whether it's valid or not. This is a correctness/performance 
  // tradeoff: building without a preamble is slow, and completion is latency-sensitive.


================
Comment at: clangd/Compiler.h:39
+/// be consumed by the FrontendAction as it will have a pointer to the MainFile
+/// buffer that will only be deleted if BeginSourceFile is called.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
----------------
bkramer wrote:
> This comment is somewhat messy now. Can you rephrase it a bit?
+1, this has grown a bit unwieldy. Maybe:
  /// Creates a compiler which will build the provided file.
  /// The preamble will not be checked for validity - caller should do that if needed.
  /// The returned compiler's VFS may differ due to command-line flags.
  /// The returned value must be consumed by a FrontendAction to avoid leaking MainFile.
  /// May return null in error cases.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41991