[clangd] Store references instead of unique_ptrs in ClangdServer.

[ilya-biryukov]

Summary

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.

[krasimir]

clangd/ClangdLSPServer.h

75 ↗

(On Diff #102333)

This approach is still inflexible. I'd make ClangdLSPServer : public DiagnosticsConsumer and pass references to the CompilationDatabase and to the FileSystemProvider to its constructor. I'd create these two in ClangdMain.cpp.

[krasimir]

But, of course, that can be done in a separate commit.

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

 //===--- ClangdLSPServer.h - LSP server --------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDLSPSERVER_H
 #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"
 
 namespace clang {
 namespace clangd {
 
 class JSONOutput;
 
 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
 class ClangdLSPServer {
 public:
   ClangdLSPServer(JSONOutput &Out, bool RunSynchronously);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   void run(std::istream &In);
 
 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);
 
   /// Function that will be called on a separate thread when diagnostics are
   /// ready. Sends the Dianostics to LSP client via Out.writeMessage and caches
   /// corresponding fixits in the FixItsMap.
   void consumeDiagnostics(PathRef File,
                           std::vector<DiagWithFixIts> Diagnostics);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
   /// Language Server client.
   /// It's used to break out of the LSP parsing loop.
   bool IsDone = false;
 
   std::mutex FixItsMutex;
   typedef std::map<clangd::Diagnostic, std::vector<clang::tooling::Replacement>>
       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.
   ClangdServer Server;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

replacementsToEdits(StringRef Code,
   if (!Edits.empty())
     Edits.pop_back();
 
   return Edits;
 }
 
 } // namespace
 
-class ClangdLSPServer::LSPDiagnosticsConsumer : public DiagnosticsConsumer {
-public:
-  LSPDiagnosticsConsumer(ClangdLSPServer &Server) : Server(Server) {}
+ClangdLSPServer::LSPDiagnosticsConsumer::LSPDiagnosticsConsumer(
+    ClangdLSPServer &Server)
+    : Server(Server) {}
 
-  virtual void onDiagnosticsReady(PathRef File,
-                                  Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
-    Server.consumeDiagnostics(File, Diagnostics.Value);
-  }
+void ClangdLSPServer::LSPDiagnosticsConsumer::onDiagnosticsReady(
+    PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
 
-private:
-  ClangdLSPServer &Server;
-};
-
 class ClangdLSPServer::LSPProtocolCallbacks : public ProtocolCallbacks {
 public:
   LSPProtocolCallbacks(ClangdLSPServer &LangServer) : LangServer(LangServer) {}
 
   void onInitialize(StringRef ID, JSONOutput &Out) override;
   void onShutdown(JSONOutput &Out) override;
   void onDocumentDidOpen(DidOpenTextDocumentParams Params,
                          JSONOutput &Out) override;
void ClangdLSPServer::LSPProtocolCallbacks::onCompletion(
   if (!Completions.empty())
     Completions.pop_back();
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":[)" + Completions + R"(]})");
 }
 
 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");
 
   // Set up JSONRPCDispatcher.
   LSPProtocolCallbacks Callbacks(*this);
   JSONRPCDispatcher Dispatcher(llvm::make_unique<Handler>(Out));
   regiterCallbackHandlers(Dispatcher, Out, Callbacks);

clang-tools-extra/trunk/clangd/ClangdServer.h

private:
   std::condition_variable RequestCV;
 };
 
 /// Provides API to manage ASTs for a collection of C++ files and request
 /// various language features(currently, only codeCompletion and async
 /// 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
   /// separate thread. When the parsing is complete, DiagConsumer passed in
   /// constructor will receive onDiagnosticsReady callback.
   void addDocument(PathRef File, StringRef Contents);
   /// Remove \p File from list of tracked files, schedule a request to free
   /// resources associated with it.
public:
   std::string getDocument(PathRef File);
 
   /// Only for testing purposes.
   /// Waits until all requests to worker thread are finished and dumps AST for
   /// \p File. \p File must be in the list of added documents.
   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;
   // WorkScheduler has to be the last member, because its destructor has to be
   // called before all other members to stop the worker thread that references
   // ClangdServer
   ClangdScheduler WorkScheduler;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/ClangdServer.cpp

void ClangdScheduler::addToEnd(std::function<void()> Request) {
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     RequestQueue.push_back(Request);
   }
   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) {}
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents) {
   DocVersion Version = DraftMgr.updateDraft(File, Contents);
   Path FileStr = File;
   WorkScheduler.addToFront([this, FileStr, Version]() {
     auto FileContents = DraftMgr.getDraft(FileStr);
     if (FileContents.Version != Version)
       return; // This request is outdated, do nothing
 
     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));
         });
   });
 }
 
 void ClangdServer::removeDocument(PathRef File) {
   auto Version = DraftMgr.removeDraft(File);
   Path FileStr = File;
if (!OverridenContents) {
     assert(FileContents.Draft &&
            "codeComplete is called for non-added document");
 
     DraftStorage = std::move(*FileContents.Draft);
     OverridenContents = DraftStorage;
   }
 
   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);
                   });
   return make_tagged(std::move(Result), TaggedFS.Tag);
 }
 
 std::vector<tooling::Replacement> ClangdServer::formatRange(PathRef File,

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

std::string replacePtrsInDump(std::string const &Dump) {
   return Result;
 }
 
 std::string dumpASTWithoutMemoryLocs(ClangdServer &Server, PathRef File) {
   auto DumpWithMemLocs = Server.dumpAST(File);
   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 {
 protected:
   SmallString<16> getVirtualTestRoot() {
 #ifdef LLVM_ON_WIN32
     return SmallString<16>("C:\\clangd-test");
 #else
     return SmallString<16>("/clangd-test");
 #endif
   }
 
   llvm::SmallString<32> getVirtualTestFilePath(PathRef File) {
     assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
 
     llvm::SmallString<32> Path;
     llvm::sys::path::append(Path, getVirtualTestRoot(), File);
     return Path;
   }
 
   std::string parseSourceAndDumpAST(
       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),
+    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;
   }
 };
 
 TEST_F(ClangdVFSTest, Parse) {
   // FIXME: figure out a stable format for AST dumps, so that we can check the
   // output of the dump itself is equal to the expected one, not just that it's
   // different.

 )cpp";
   parseSourceAndDumpAST("foo.cpp", SourceContents, {{"foo.h", ""}},
                         /*ExpectErrors=*/true);
   parseSourceAndDumpAST("foo.cpp", SourceContents, {{"foo.h", "int a;"}},
                         /*ExpectErrors=*/false);
 }
 
 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),
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, DiagConsumer, FS,
                       /*RunSynchronously=*/false);
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
 int b = a;
 )cpp";
 
   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),
+  ClangdServer Server(CDB, DiagConsumer, FS,
                       /*RunSynchronously=*/false);
 
   const auto SourceContents = R"cpp(
 #include "foo.h"
 int b = a;
 )cpp";
 
   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),
+  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 {
 protected:
   bool ContainsItem(std::vector<CompletionItem> const &Items, StringRef Name) {
     for (const auto &Item : Items) {
       if (Item.insertText == Name)
         return true;
     }
     return false;
   }
 };
 
 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");
   const auto SourceContents = R"cpp(
 int aba;
 int b =   ;
 )cpp";
 
   const auto OverridenSourceContents = R"cpp(
 int cbc;
 int b =   ;
 )cpp";
   // Complete after '=' sign. We need to be careful to keep the SourceContents'
   // size the same.
   // We complete on the 3rd line (2nd in zero-based numbering), because raw
   // 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);
 
   {
     auto CodeCompletionResults1 =
         Server.codeComplete(FooCpp, CompletePos, None).Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResults1, "aba"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResults1, "cbc"));