[PATCH] D50695: [clangd] Always use the latest preamble

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar.

Fix an inconsistent behavior of using `LastBuiltPreamble`/`NewPreamble`
in TUScheduler (see the test for details), AST should always use
NewPreamble. This patch makes LastBuiltPreamble always point to
NewPreamble.

Preamble rarely fails to build, even there are errors in headers, so we
assume it would not cause performace issue for code completion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50695

Files:
  clangd/TUScheduler.cpp
  unittests/clangd/XRefsTests.cpp


Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1021,6 +1021,65 @@
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinitoin, WithPreamble) {
+  // Test stragety: AST should always use the latest preamble instead of last
+  // good preamble.
+  MockFSProvider FS;
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto FooCppUri = URIForFile{FooCpp};
+  // The trigger locations must be the same.
+  Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
+  Annotations FooWithoutHeader(R"cpp(double    [[fo^o]]();)cpp");
+
+  FS.Files[FooCpp] = FooWithHeader.code();
+
+  auto FooH = testPath("foo.h");
+  auto FooHUri = URIForFile{FooH};
+  Annotations FooHeader(R"cpp([[]])cpp");
+  FS.Files[FooH] = FooHeader.code();
+
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // GoToDefinition goes to a #include file: the result comes from the preamble.
+  Server.findDefinitions(
+      FooCpp, FooWithHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc, ElementsAre(Location{FooHUri, FooHeader.range()}));
+      });
+
+  // Only preamble is built, and no AST is built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
+  // We build AST here, and it should use the latest preamble rather than the
+  // stale one.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc,
+                    ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+
+
+  // Reset test environment.
+  runAddDocument(Server, FooCpp, FooWithHeader.code());
+  // Both preamble and AST are built in this request.
+  Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
+  // Use the AST being built in above request.
+  Server.findDefinitions(
+      FooCpp, FooWithoutHeader.point(),
+      [&](llvm::Expected<std::vector<Location>> Loc) {
+        ASSERT_TRUE(bool(Loc));
+        EXPECT_THAT(*Loc,
+                    ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      });
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Wait for GoToDefinition";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -370,8 +370,8 @@
     bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      if (NewPreamble)
-        LastBuiltPreamble = NewPreamble;
+      // Always use the NewPreamble.
+      LastBuiltPreamble = NewPreamble;
     }
     // Before doing the expensive AST reparse, we want to release our reference
     // to the old preamble, so it can be freed if there are no other references


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50695.160540.patch
Type: text/x-patch
Size: 3288 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180814/66e20b0e/attachment.bin>