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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline 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
----------------
sammccall wrote:
> 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.
Looks much better, thanks.


================
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(
----------------
sammccall wrote:
> 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.
> 
I rewrote the comment before reading your suggestion, it should be better now.
If it's still unclear, let me know.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41991