[PATCH] D52071: [clangd] Don't override the preamble while completing inside it, it doesn't work.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric.

To stay fast, enable single-file-mode instead. This is fine since completions
in the preamble are simple.

The net effect for now is to suppress the spurious TopLevel completions when
completing inside the preamble.
Once Sema has include directive completion, this will be more important.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52071

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -657,6 +657,22 @@
               UnorderedElementsAre(Named("local"), Named("preamble")));
 }
 
+// This verifies that we get normal preprocessor completions in the preamble.
+// This is a regression test for an old bug: if we override the preamble and
+// try to complete inside it, clang kicks our completion point just outside the
+// preamble, resulting in always getting top-level completions.
+TEST(CompletionTest, CompletionInPreamble) {
+  EXPECT_THAT(completions(R"cpp(
+    #ifnd^ef FOO_H_
+    #define BAR_H_
+    #include <bar.h>
+    int foo() {}
+    #endif
+    )cpp")
+                  .Completions,
+              ElementsAre(Named("ifndef")));
+};
+
 TEST(CompletionTest, DynamicIndexMultiFile) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -40,6 +40,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Index/USRGeneration.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -1053,11 +1054,19 @@
   // 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.
+  // However, if we're completing *inside* the preamble section of the draft,
+  // overriding the preamble will break sema completion. Fortunately we can just
+  // skip all includes in this case; these completions are really simple.
+  bool CompletingInPreamble =
+      ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0).Size >
+      *Offset;
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
   auto Clang = prepareCompilerInstance(
-      std::move(CI), Input.Preamble, std::move(ContentsBuffer),
-      std::move(Input.PCHs), std::move(Input.VFS), DummyDiagsConsumer);
+      std::move(CI), CompletingInPreamble ? nullptr : Input.Preamble,
+      std::move(ContentsBuffer), std::move(Input.PCHs), std::move(Input.VFS),
+      DummyDiagsConsumer);
+  Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble;
   Clang->setCodeCompletionConsumer(Consumer.release());
 
   SyntaxOnlyAction Action;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D52071.165421.patch
Type: text/x-patch
Size: 2662 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180914/15c66363/attachment-0001.bin>