[clang-tools-extra] r340001 - [clangd] Always use the latest preamble

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Aug 17 01:15:22 2018
New Revision: 340001

URL: http://llvm.org/viewvc/llvm-project?rev=340001&view=rev
Log:
[clangd] Always use the latest preamble

Summary:
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.

Reviewers: ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, cfe-commits

Differential Revision: https://reviews.llvm.org/D50695

Modified:
    clang-tools-extra/trunk/clangd/TUScheduler.cpp
    clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=340001&r1=340000&r2=340001&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Fri Aug 17 01:15:22 2018
@@ -370,8 +370,7 @@ void ASTWorker::update(
     bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
     {
       std::lock_guard<std::mutex> Lock(Mutex);
-      if (NewPreamble)
-        LastBuiltPreamble = 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

Modified: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp?rev=340001&r1=340000&r2=340001&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp Fri Aug 17 01:15:22 2018
@@ -1021,6 +1021,51 @@ TEST(GoToInclude, All) {
   EXPECT_THAT(*Locations, IsEmpty());
 }
 
+TEST(GoToDefinition, 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.
+  EXPECT_THAT(
+      cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
+      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.
+  EXPECT_THAT(
+      cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+
+  // 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.
+  EXPECT_THAT(
+      cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
+      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang