ilya-biryukov created this revision.
ClangdServer was owning objects passed to it in constructor for no good reason.
Lots of stuff was moved from the heap to the stack thanks to this change.
https://reviews.llvm.org/D34148
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -214,11 +214,6 @@
return replacePtrsInDump(DumpWithMemLocs);
}
-template <class T>
-std::unique_ptr<T> getAndMove(std::unique_ptr<T> Ptr, T *&Output) {
- Output = Ptr.get();
- return Ptr;
-}
} // namespace
class ClangdVFSTest : public ::testing::Test {
@@ -243,22 +238,19 @@
PathRef SourceFileRelPath, StringRef SourceContents,
std::vector<std::pair<PathRef, StringRef>> ExtraFiles = {},
bool ExpectErrors = false) {
- MockFSProvider *FS;
- ErrorCheckingDiagConsumer *DiagConsumer;
- ClangdServer Server(
- llvm::make_unique<MockCompilationDatabase>(),
- getAndMove(llvm::make_unique<ErrorCheckingDiagConsumer>(),
- DiagConsumer),
- getAndMove(llvm::make_unique<MockFSProvider>(), FS),
- /*RunSynchronously=*/false);
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, DiagConsumer, FS,
+ /*RunSynchronously=*/false);
for (const auto &FileWithContents : ExtraFiles)
- FS->Files[getVirtualTestFilePath(FileWithContents.first)] =
+ FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
FileWithContents.second;
auto SourceFilename = getVirtualTestFilePath(SourceFileRelPath);
Server.addDocument(SourceFilename, SourceContents);
auto Result = dumpASTWithoutMemoryLocs(Server, SourceFilename);
- EXPECT_EQ(ExpectErrors, DiagConsumer->hadErrorInLastDiags());
+ EXPECT_EQ(ExpectErrors, DiagConsumer.hadErrorInLastDiags());
return Result;
}
};
@@ -299,13 +291,11 @@
}
TEST_F(ClangdVFSTest, Reparse) {
- MockFSProvider *FS;
- ErrorCheckingDiagConsumer *DiagConsumer;
- ClangdServer Server(
- llvm::make_unique<MockCompilationDatabase>(),
- getAndMove(llvm::make_unique<ErrorCheckingDiagConsumer>(), DiagConsumer),
- getAndMove(llvm::make_unique<MockFSProvider>(), FS),
- /*RunSynchronously=*/false);
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, DiagConsumer, FS,
+ /*RunSynchronously=*/false);
const auto SourceContents = R"cpp(
#include "foo.h"
@@ -315,34 +305,32 @@
auto FooCpp = getVirtualTestFilePath("foo.cpp");
auto FooH = getVirtualTestFilePath("foo.h");
- FS->Files[FooH] = "int a;";
- FS->Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = "int a;";
+ FS.Files[FooCpp] = SourceContents;
Server.addDocument(FooCpp, SourceContents);
auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_FALSE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
Server.addDocument(FooCpp, "");
auto DumpParseEmpty = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_FALSE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
Server.addDocument(FooCpp, SourceContents);
auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_FALSE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
EXPECT_EQ(DumpParse1, DumpParse2);
EXPECT_NE(DumpParse1, DumpParseEmpty);
}
TEST_F(ClangdVFSTest, ReparseOnHeaderChange) {
- MockFSProvider *FS;
- ErrorCheckingDiagConsumer *DiagConsumer;
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
- ClangdServer Server(
- llvm::make_unique<MockCompilationDatabase>(),
- getAndMove(llvm::make_unique<ErrorCheckingDiagConsumer>(), DiagConsumer),
- getAndMove(llvm::make_unique<MockFSProvider>(), FS),
- /*RunSynchronously=*/false);
+ ClangdServer Server(CDB, DiagConsumer, FS,
+ /*RunSynchronously=*/false);
const auto SourceContents = R"cpp(
#include "foo.h"
@@ -352,50 +340,47 @@
auto FooCpp = getVirtualTestFilePath("foo.cpp");
auto FooH = getVirtualTestFilePath("foo.h");
- FS->Files[FooH] = "int a;";
- FS->Files[FooCpp] = SourceContents;
+ FS.Files[FooH] = "int a;";
+ FS.Files[FooCpp] = SourceContents;
Server.addDocument(FooCpp, SourceContents);
auto DumpParse1 = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_FALSE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
- FS->Files[FooH] = "";
+ FS.Files[FooH] = "";
Server.forceReparse(FooCpp);
auto DumpParseDifferent = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_TRUE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
- FS->Files[FooH] = "int a;";
+ FS.Files[FooH] = "int a;";
Server.forceReparse(FooCpp);
auto DumpParse2 = dumpASTWithoutMemoryLocs(Server, FooCpp);
- EXPECT_FALSE(DiagConsumer->hadErrorInLastDiags());
+ EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
EXPECT_EQ(DumpParse1, DumpParse2);
EXPECT_NE(DumpParse1, DumpParseDifferent);
}
TEST_F(ClangdVFSTest, CheckVersions) {
- MockFSProvider *FS;
- ErrorCheckingDiagConsumer *DiagConsumer;
-
- ClangdServer Server(
- llvm::make_unique<MockCompilationDatabase>(),
- getAndMove(llvm::make_unique<ErrorCheckingDiagConsumer>(), DiagConsumer),
- getAndMove(llvm::make_unique<MockFSProvider>(), FS),
- /*RunSynchronously=*/true);
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
+ ClangdServer Server(CDB, DiagConsumer, FS,
+ /*RunSynchronously=*/true);
auto FooCpp = getVirtualTestFilePath("foo.cpp");
const auto SourceContents = "int a;";
- FS->Files[FooCpp] = SourceContents;
- FS->Tag = "123";
+ FS.Files[FooCpp] = SourceContents;
+ FS.Tag = "123";
Server.addDocument(FooCpp, SourceContents);
- EXPECT_EQ(DiagConsumer->lastVFSTag(), FS->Tag);
- EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS->Tag);
+ EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
+ EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
- FS->Tag = "321";
+ FS.Tag = "321";
Server.addDocument(FooCpp, SourceContents);
- EXPECT_EQ(DiagConsumer->lastVFSTag(), FS->Tag);
- EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS->Tag);
+ EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
+ EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS.Tag);
}
class ClangdCompletionTest : public ClangdVFSTest {
@@ -410,11 +395,11 @@
};
TEST_F(ClangdCompletionTest, CheckContentsOverride) {
- MockFSProvider *FS;
+ MockFSProvider FS;
+ ErrorCheckingDiagConsumer DiagConsumer;
+ MockCompilationDatabase CDB;
- ClangdServer Server(llvm::make_unique<MockCompilationDatabase>(),
- llvm::make_unique<ErrorCheckingDiagConsumer>(),
- getAndMove(llvm::make_unique<MockFSProvider>(), FS),
+ ClangdServer Server(CDB, DiagConsumer, FS,
/*RunSynchronously=*/false);
auto FooCpp = getVirtualTestFilePath("foo.cpp");
@@ -433,7 +418,7 @@
// string literal of the SourceContents starts with a newline(it's easy to
// miss).
Position CompletePos = {2, 8};
- FS->Files[FooCpp] = SourceContents;
+ FS.Files[FooCpp] = SourceContents;
Server.addDocument(FooCpp, SourceContents);
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -136,10 +136,9 @@
/// diagnostics for tracked files).
class ClangdServer {
public:
- ClangdServer(std::unique_ptr<GlobalCompilationDatabase> CDB,
- std::unique_ptr<DiagnosticsConsumer> DiagConsumer,
- std::unique_ptr<FileSystemProvider> FSProvider,
- bool RunSynchronously);
+ ClangdServer(GlobalCompilationDatabase &CDB,
+ DiagnosticsConsumer &DiagConsumer,
+ FileSystemProvider &FSProvider, bool RunSynchronously);
/// Add a \p File to the list of tracked C++ files or update the contents if
/// \p File is already tracked. Also schedules parsing of the AST for it on a
@@ -181,9 +180,9 @@
std::string dumpAST(PathRef File);
private:
- std::unique_ptr<GlobalCompilationDatabase> CDB;
- std::unique_ptr<DiagnosticsConsumer> DiagConsumer;
- std::unique_ptr<FileSystemProvider> FSProvider;
+ GlobalCompilationDatabase &CDB;
+ DiagnosticsConsumer &DiagConsumer;
+ FileSystemProvider &FSProvider;
DraftStore DraftMgr;
ClangdUnitStore Units;
std::shared_ptr<PCHContainerOperations> PCHs;
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -138,12 +138,11 @@
RequestCV.notify_one();
}
-ClangdServer::ClangdServer(std::unique_ptr<GlobalCompilationDatabase> CDB,
- std::unique_ptr<DiagnosticsConsumer> DiagConsumer,
- std::unique_ptr<FileSystemProvider> FSProvider,
+ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
+ DiagnosticsConsumer &DiagConsumer,
+ FileSystemProvider &FSProvider,
bool RunSynchronously)
- : CDB(std::move(CDB)), DiagConsumer(std::move(DiagConsumer)),
- FSProvider(std::move(FSProvider)),
+ : CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
PCHs(std::make_shared<PCHContainerOperations>()),
WorkScheduler(RunSynchronously) {}
@@ -157,11 +156,11 @@
assert(FileContents.Draft &&
"No contents inside a file that was scheduled for reparse");
- auto TaggedFS = FSProvider->getTaggedFileSystem();
+ auto TaggedFS = FSProvider.getTaggedFileSystem();
Units.runOnUnit(
- FileStr, *FileContents.Draft, *CDB, PCHs, TaggedFS.Value,
+ FileStr, *FileContents.Draft, CDB, PCHs, TaggedFS.Value,
[&](ClangdUnit const &Unit) {
- DiagConsumer->onDiagnosticsReady(
+ DiagConsumer.onDiagnosticsReady(
FileStr, make_tagged(Unit.getLocalDiagnostics(), TaggedFS.Tag));
});
});
@@ -198,11 +197,11 @@
}
std::vector<CompletionItem> Result;
- auto TaggedFS = FSProvider->getTaggedFileSystem();
+ auto TaggedFS = FSProvider.getTaggedFileSystem();
// It would be nice to use runOnUnitWithoutReparse here, but we can't
// guarantee the correctness of code completion cache here if we don't do the
// reparse.
- Units.runOnUnit(File, *OverridenContents, *CDB, PCHs, TaggedFS.Value,
+ Units.runOnUnit(File, *OverridenContents, CDB, PCHs, TaggedFS.Value,
[&](ClangdUnit &Unit) {
Result = Unit.codeComplete(*OverridenContents, Pos,
TaggedFS.Value);
Index: clangd/ClangdLSPServer.h
===================================================================
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -11,6 +11,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDLSPSERVER_H
#include "ClangdServer.h"
+#include "GlobalCompilationDatabase.h"
#include "Path.h"
#include "Protocol.h"
#include "clang/Tooling/Core/Replacement.h"
@@ -34,7 +35,17 @@
private:
class LSPProtocolCallbacks;
- class LSPDiagnosticsConsumer;
+ class LSPDiagnosticsConsumer : public DiagnosticsConsumer {
+ public:
+ LSPDiagnosticsConsumer(ClangdLSPServer &Server);
+
+ virtual void
+ onDiagnosticsReady(PathRef File,
+ Tagged<std::vector<DiagWithFixIts>> Diagnostics);
+
+ private:
+ ClangdLSPServer &Server;
+ };
std::vector<clang::tooling::Replacement>
getFixIts(StringRef File, const clangd::Diagnostic &D);
@@ -56,6 +67,13 @@
DiagnosticToReplacementMap;
/// Caches FixIts per file and diagnostics
llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
+
+ // Various ClangdServer parameters go here. It's important they're created
+ // before ClangdServer.
+ DirectoryBasedGlobalCompilationDatabase CDB;
+ LSPDiagnosticsConsumer DiagConsumer;
+ RealFileSystemProvider FSProvider;
+
// Server must be the last member of the class to allow its destructor to exit
// the worker thread that may otherwise run an async callback on partially
// destructed instance of ClangdLSPServer.
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -38,18 +38,14 @@
} // namespace
-class ClangdLSPServer::LSPDiagnosticsConsumer : public DiagnosticsConsumer {
-public:
- LSPDiagnosticsConsumer(ClangdLSPServer &Server) : Server(Server) {}
-
- virtual void onDiagnosticsReady(PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
- Server.consumeDiagnostics(File, Diagnostics.Value);
- }
+ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
+ ClangdLSPServer &Server)
+ : Server(Server) {}
-private:
- ClangdLSPServer &Server;
-};
+void ClangdLSPServer::LSPDiagnosticsConsumer::onDiagnosticsReady(
+ PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
+ Server.consumeDiagnostics(File, Diagnostics.Value);
+}
class ClangdLSPServer::LSPProtocolCallbacks : public ProtocolCallbacks {
public:
@@ -196,11 +192,8 @@
}
ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, bool RunSynchronously)
- : Out(Out),
- Server(llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(),
- llvm::make_unique<LSPDiagnosticsConsumer>(*this),
- llvm::make_unique<RealFileSystemProvider>(),
- RunSynchronously) {}
+ : Out(Out), DiagConsumer(*this),
+ Server(CDB, DiagConsumer, FSProvider, RunSynchronously) {}
void ClangdLSPServer::run(std::istream &In) {
assert(!IsDone && "Run was called before");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
