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