[clang-tools-extra] r305298 - [clangd] Store references instead of unique_ptrs in ClangdServer.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 13 08:59:43 PDT 2017
Author: ibiryukov
Date: Tue Jun 13 10:59:43 2017
New Revision: 305298
URL: http://llvm.org/viewvc/llvm-project?rev=305298&view=rev
Log:
[clangd] Store references instead of unique_ptrs in ClangdServer.
Summary:
ClangdServer owned 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.
Reviewers: krasimir
Reviewed By: krasimir
Subscribers: klimek, cfe-commits
Differential Revision: https://reviews.llvm.org/D34148
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=305298&r1=305297&r2=305298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jun 13 10:59:43 2017
@@ -38,18 +38,14 @@ replacementsToEdits(StringRef Code,
} // 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);
- }
-
-private:
- ClangdLSPServer &Server;
-};
+ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
+ ClangdLSPServer &Server)
+ : Server(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 @@ void ClangdLSPServer::LSPProtocolCallbac
}
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");
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=305298&r1=305297&r2=305298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Tue Jun 13 10:59:43 2017
@@ -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 @@ public:
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 @@ private:
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.
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=305298&r1=305297&r2=305298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Jun 13 10:59:43 2017
@@ -138,12 +138,11 @@ void ClangdScheduler::addToEnd(std::func
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 @@ void ClangdServer::addDocument(PathRef F
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 @@ ClangdServer::codeComplete(PathRef File,
}
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);
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=305298&r1=305297&r2=305298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Tue Jun 13 10:59:43 2017
@@ -136,10 +136,9 @@ private:
/// 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 @@ public:
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;
Modified: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp?rev=305298&r1=305297&r2=305298&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp Tue Jun 13 10:59:43 2017
@@ -214,11 +214,6 @@ std::string dumpASTWithoutMemoryLocs(Cla
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 @@ protected:
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 @@ int b = a;
}
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 @@ int b = a;
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 @@ int b = a;
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 @@ protected:
};
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 @@ int b = ;
// 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);
More information about the cfe-commits
mailing list