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

[Phabricator via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL322854: [clangd] Always use preamble (even stale) for code completion (authored by ibiryukov, committed by ).
Herald added subscribers: llvm-commits, ioeric.

Repository:
  rL LLVM

https://reviews.llvm.org/D41991

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/Compiler.cpp
  clang-tools-extra/trunk/clangd/Compiler.h


Index: clang-tools-extra/trunk/clangd/Compiler.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/Compiler.cpp
+++ clang-tools-extra/trunk/clangd/Compiler.cpp
@@ -14,13 +14,6 @@
 namespace clang {
 namespace clangd {
 
-/// Creates a CompilerInstance from \p CI, with main buffer overriden to \p
-/// Buffer and arguments to read the PCH from \p Preamble, if \p Preamble is not
-/// null. Note that vfs::FileSystem inside returned instance may differ from \p
-/// VFS if additional file remapping were set in command-line arguments.
-/// On some errors, returns null. When non-null value is returned, it's expected
-/// to be consumed by the FrontendAction as it will have a pointer to the \p
-/// Buffer that will only be deleted if BeginSourceFile is called.
 std::unique_ptr<CompilerInstance>
 prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
                         const PrecompiledPreamble *Preamble,
@@ -36,7 +29,7 @@
   // NOTE: we use Buffer.get() when adding remapped files, so we have to make
   // sure it will be released if no error is emitted.
   if (Preamble) {
-    Preamble->AddImplicitPreamble(*CI, VFS, Buffer.get());
+    Preamble->OverridePreamble(*CI, VFS, Buffer.get());
   } else {
     CI->getPreprocessorOpts().addRemappedFile(
         CI->getFrontendOpts().Inputs[0].getFile(), Buffer.get());
Index: clang-tools-extra/trunk/clangd/Compiler.h
===================================================================
--- clang-tools-extra/trunk/clangd/Compiler.h
+++ clang-tools-extra/trunk/clangd/Compiler.h
@@ -28,13 +28,16 @@
                         const clang::Diagnostic &Info) override {}
 };
 
-/// Creates a CompilerInstance with the main file contens overridden.
-/// The preamble will be reused unless it is null.
-/// Note that the vfs::FileSystem inside returned instance may differ if
-/// additional file remappings occur in command-line arguments.
-/// On some errors, returns null. When non-null value is returned, it's expected
-/// to be consumed by the FrontendAction as it will have a pointer to the
-/// MainFile buffer that will only be deleted if BeginSourceFile is called.
+/// Creates a compiler instance, configured so that:
+///   - Contents of the parsed file are remapped to \p MainFile.
+///   - Preamble is overriden to use PCH passed to this function. It means the
+///     changes to the preamble headers or files included in the preamble are
+///     not visible to this compiler instance.
+///   - vfs::FileSystem is used for all underlying file accesses. The actual
+///     vfs used by the compiler may be an overlay over the passed vfs.
+/// Returns null on errors. When non-null value is returned, it is expected to
+/// be consumed by FrontendAction::BeginSourceFile to properly destroy \p
+/// MainFile.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
     std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
     std::unique_ptr<llvm::MemoryBuffer> MainFile,
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -517,14 +517,18 @@
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Contents, FileName);
 
-  // Attempt to reuse the PCH from precompiled preamble, if it was built.
+  // 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.
   if (Preamble) {
     auto Bounds =
         ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
-    if (!Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, VFS.get()))
-      Preamble = nullptr;
+    // FIXME(ibiryukov): Remove this call to CanReuse() after we'll fix
+    // clients relying on getting stats for preamble files during code
+    // completion.
+    // Note that results of CanReuse() are ignored, see the comment above.
+    Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, VFS.get());
   }
-
   auto Clang = prepareCompilerInstance(
       std::move(CI), Preamble, std::move(ContentsBuffer), std::move(PCHs),
       std::move(VFS), DummyDiagsConsumer);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41991.130409.patch
Type: text/x-patch
Size: 4367 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180118/638c3636/attachment.bin>