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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: bkramer, sammccall.
Herald added a subscriber: klimek.

This improves performance of code completion, because we avoid stating
the files from the preamble and never attempt to parse the files
without using the preamble if it's provided.

However, the change comes at a cost of sometimes providing incorrect
results when doing code completion after making actually considerable
changes to the files used in the preamble or the preamble itself.
Eventually the preamble will get rebuilt and code completion will
be providing the correct results.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41991

Files:
  clangd/CodeComplete.cpp
  clangd/Compiler.cpp
  clangd/Compiler.h


Index: clangd/Compiler.h
===================================================================
--- clangd/Compiler.h
+++ clangd/Compiler.h
@@ -29,12 +29,14 @@
 };
 
 /// 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.
+/// The preamble will be set up via PrecompiledPreamble::OverridePreamble
+/// without doing any extra checks, the callers are responsible for checking
+/// PrecompiledPreamble::CanReuse if they need to make sure only up-to-date
+/// preambles are used . 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.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
     std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
     std::unique_ptr<llvm::MemoryBuffer> MainFile,
Index: clangd/Compiler.cpp
===================================================================
--- clangd/Compiler.cpp
+++ clangd/Compiler.cpp
@@ -36,7 +36,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: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -519,14 +519,11 @@
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Contents, FileName);
 
-  // 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
+  // trade correctness in some cases (preamble has not been rebuilt after
+  // changes to a file) for performance. Eventually the preamble will be rebuilt
+  // and code completion will produce correct results.
   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.129614.patch
Type: text/x-patch
Size: 3157 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180112/21abeb1f/attachment-0001.bin>