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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
