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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 17 08:38:38 PST 2018

ilya-biryukov updated this revision to Diff 130193.
ilya-biryukov added a comment.

- Rewrote the comment for prepareCompilerInstance
- Call CanReuse() and discard its results. We have clients that rely on having the stat() calls for files from preamble

  rCTE Clang Tools Extra



Index: clangd/Compiler.h
--- clangd/Compiler.h
+++ 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 passe vfs.
+/// Returns null on errors. When non-null value is returned, it is expected to
+/// be consumed by FrontendAction::BeginSourceFile to properly destroy buffer
+/// for \p MainFile.
 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
@@ -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.
 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->getFrontendOpts().Inputs[0].getFile(), Buffer.get());
Index: clangd/CodeComplete.cpp
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -517,14 +517,20 @@
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Contents, FileName);
-  // Attempt to reuse the PCH from precompiled preamble, if it was built.
+  // Note that we delibirately allow outdated preambles. Not using the preamble
+  // makes code completion much slower and we feel that 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.
   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.130193.patch
Type: text/x-patch
Size: 4358 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180117/79f99787/attachment-0001.bin>

More information about the cfe-commits mailing list