[clangd] simplify ClangdLSPServer by private-inheriting callback interfaces. NFC

[sammccall]

Summary

There doesn't seem to be any real separation between the current three objects.
Feel free to reject this if you find the current style valuable, though.

(Mostly I'm just looking around for cleanups to help me understand the code).

[ilya-biryukov]

LGTM.

PS I actually remember arguing to keep those classes separate in the original review :-)

[ilya-biryukov]

clangd/ClangdLSPServer.h

41 ↗

(On Diff #117178)

NIT: Missing full stop at the end of the comment.

46 ↗

(On Diff #117178)

NIT: Missing full stop at the end of the comment.

[sammccall]

• Address review comments

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 "ProtocolHandlers.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 
 namespace clang {
 namespace clangd {
 
 class JSONOutput;
 
 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
-class ClangdLSPServer {
+class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                   bool SnippetCompletions,
                   llvm::Optional<StringRef> ResourceDir);
 
   /// 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 : public DiagnosticsConsumer {
-  public:
-    LSPDiagnosticsConsumer(ClangdLSPServer &Server);
-
-    virtual void
-    onDiagnosticsReady(PathRef File,
-                       Tagged<std::vector<DiagWithFixIts>> Diagnostics);
+  // Implement DiagnosticsConsumer.
+                     Tagged<std::vector<DiagWithFixIts>> Diagnostics) override;
 
-  private:
-    ClangdLSPServer &Server;
-  };
+  // Implement ProtocolCallbacks.
+  void onInitialize(StringRef ID, InitializeParams IP,
+                    JSONOutput &Out) override;
+  void onShutdown(JSONOutput &Out) override;
+  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
+                         JSONOutput &Out) override;
+  void onDocumentDidChange(DidChangeTextDocumentParams Params,
+                           JSONOutput &Out) override;
+  void onDocumentDidClose(DidCloseTextDocumentParams Params,
+                          JSONOutput &Out) override;
+  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
+                                  StringRef ID, JSONOutput &Out) override;
+  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
+                                 StringRef ID, JSONOutput &Out) override;
+  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
+                            JSONOutput &Out) override;
+  void onCodeAction(CodeActionParams Params, StringRef ID,
+                    JSONOutput &Out) override;
+  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
+                    JSONOutput &Out) override;
+  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
+                        JSONOutput &Out) override;
+  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+                            JSONOutput &Out) override;
 
   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,
private:
   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

 //===--- ClangdLSPServer.cpp - LSP server ------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
-#include "ProtocolHandlers.h"
 
 using namespace clang::clangd;
 using namespace clang;
 
 namespace {
 
 std::string
 replacementsToEdits(StringRef Code,
replacementsToEdits(StringRef Code,
   if (!Edits.empty())
     Edits.pop_back();
 
   return Edits;
 }
 
 } // namespace
 
-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:
-  LSPProtocolCallbacks(ClangdLSPServer &LangServer) : LangServer(LangServer) {}
-
-  void onInitialize(StringRef ID, InitializeParams IP,
-                    JSONOutput &Out) override;
-  void onShutdown(JSONOutput &Out) override;
-  void onDocumentDidOpen(DidOpenTextDocumentParams Params,
-                         JSONOutput &Out) override;
-  void onDocumentDidChange(DidChangeTextDocumentParams Params,
-                           JSONOutput &Out) override;
-  void onDocumentDidClose(DidCloseTextDocumentParams Params,
-                          JSONOutput &Out) override;
-  void onDocumentOnTypeFormatting(DocumentOnTypeFormattingParams Params,
-                                  StringRef ID, JSONOutput &Out) override;
-  void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
-                                 StringRef ID, JSONOutput &Out) override;
-  void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
-                            JSONOutput &Out) override;
-  void onCodeAction(CodeActionParams Params, StringRef ID,
-                    JSONOutput &Out) override;
-  void onCompletion(TextDocumentPositionParams Params, StringRef ID,
-                    JSONOutput &Out) override;
-  void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
-                        JSONOutput &Out) override;
-  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
-                        JSONOutput &Out) override;                      
-
-private:
-  ClangdLSPServer &LangServer;
-};
-
-void ClangdLSPServer::LSPProtocolCallbacks::onInitialize(StringRef ID,
-                                                         InitializeParams IP,
-                                                         JSONOutput &Out) {
+void ClangdLSPServer::onInitialize(StringRef ID, InitializeParams IP,
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID +
       R"(,"result":{"capabilities":{
           "textDocumentSync": 1,
           "documentFormattingProvider": true,
           "documentRangeFormattingProvider": true,
           "documentOnTypeFormattingProvider": {"firstTriggerCharacter":"}","moreTriggerCharacter":[]},
           "codeActionProvider": true,
           "completionProvider": {"resolveProvider": false, "triggerCharacters": [".",">",":"]},
           "definitionProvider": true
         }}})");
   if (IP.rootUri && !IP.rootUri->file.empty())
-    LangServer.Server.setRootPath(IP.rootUri->file);
+    Server.setRootPath(IP.rootUri->file);
   else if (IP.rootPath && !IP.rootPath->empty())
-    LangServer.Server.setRootPath(*IP.rootPath);
+    Server.setRootPath(*IP.rootPath);
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onShutdown(JSONOutput &Out) {
-  LangServer.IsDone = true;
-}
+void ClangdLSPServer::onShutdown(JSONOutput &Out) { IsDone = true; }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidOpen(
-    DidOpenTextDocumentParams Params, JSONOutput &Out) {
+void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams Params,
+                                        JSONOutput &Out) {
   if (Params.metadata && !Params.metadata->extraFlags.empty())
-    LangServer.CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
-                                        std::move(Params.metadata->extraFlags));
-  LangServer.Server.addDocument(Params.textDocument.uri.file,
-                                Params.textDocument.text);
+    CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
+  Server.addDocument(Params.textDocument.uri.file, Params.textDocument.text);
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidChange(
-    DidChangeTextDocumentParams Params, JSONOutput &Out) {
+void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams Params,
+                                          JSONOutput &Out) {
   // We only support full syncing right now.
-  LangServer.Server.addDocument(Params.textDocument.uri.file,
-                                Params.contentChanges[0].text);
+  Server.addDocument(Params.textDocument.uri.file,
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidClose(
-    DidCloseTextDocumentParams Params, JSONOutput &Out) {
-  LangServer.Server.removeDocument(Params.textDocument.uri.file);
+void ClangdLSPServer::onDocumentDidClose(DidCloseTextDocumentParams Params,
+                                         JSONOutput &Out) {
+  Server.removeDocument(Params.textDocument.uri.file);
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentOnTypeFormatting(
+void ClangdLSPServer::onDocumentOnTypeFormatting(
     DocumentOnTypeFormattingParams Params, StringRef ID, JSONOutput &Out) {
   auto File = Params.textDocument.uri.file;
-  std::string Code = LangServer.Server.getDocument(File);
-  std::string Edits = replacementsToEdits(
-      Code, LangServer.Server.formatOnType(File, Params.position));
+  std::string Code = Server.getDocument(File);
+  std::string Edits =
+      replacementsToEdits(Code, Server.formatOnType(File, Params.position));
 
   Out.writeMessage(R"({"jsonrpc":"2.0","id":)" + ID.str() +
                    R"(,"result":[)" + Edits + R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentRangeFormatting(
+void ClangdLSPServer::onDocumentRangeFormatting(
     DocumentRangeFormattingParams Params, StringRef ID, JSONOutput &Out) {
   auto File = Params.textDocument.uri.file;
-  std::string Code = LangServer.Server.getDocument(File);
-  std::string Edits = replacementsToEdits(
-      Code, LangServer.Server.formatRange(File, Params.range));
+  std::string Code = Server.getDocument(File);
+  std::string Edits =
+      replacementsToEdits(Code, Server.formatRange(File, Params.range));
 
   Out.writeMessage(R"({"jsonrpc":"2.0","id":)" + ID.str() +
                    R"(,"result":[)" + Edits + R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onDocumentFormatting(
-    DocumentFormattingParams Params, StringRef ID, JSONOutput &Out) {
+void ClangdLSPServer::onDocumentFormatting(DocumentFormattingParams Params,
+                                           StringRef ID, JSONOutput &Out) {
   auto File = Params.textDocument.uri.file;
-  std::string Code = LangServer.Server.getDocument(File);
-  std::string Edits =
-      replacementsToEdits(Code, LangServer.Server.formatFile(File));
+  std::string Code = Server.getDocument(File);
+  std::string Edits = replacementsToEdits(Code, Server.formatFile(File));
 
   Out.writeMessage(R"({"jsonrpc":"2.0","id":)" + ID.str() +
                    R"(,"result":[)" + Edits + R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onCodeAction(
-    CodeActionParams Params, StringRef ID, JSONOutput &Out) {
+void ClangdLSPServer::onCodeAction(CodeActionParams Params, StringRef ID,
+                                   JSONOutput &Out) {
   // We provide a code action for each diagnostic at the requested location
   // which has FixIts available.
-  std::string Code =
-      LangServer.Server.getDocument(Params.textDocument.uri.file);
+  std::string Code = Server.getDocument(Params.textDocument.uri.file);
   std::string Commands;
   for (Diagnostic &D : Params.context.diagnostics) {
     std::vector<clang::tooling::Replacement> Fixes =
-        LangServer.getFixIts(Params.textDocument.uri.file, D);
+        getFixIts(Params.textDocument.uri.file, D);
     std::string Edits = replacementsToEdits(Code, Fixes);
 
     if (!Edits.empty())
       Commands +=
           R"({"title":"Apply FixIt ')" + llvm::yaml::escape(D.message) +
           R"('", "command": "clangd.applyFix", "arguments": [")" +
           llvm::yaml::escape(Params.textDocument.uri.uri) +
           R"(", [)" + Edits +
           R"(]]},)";
   }
   if (!Commands.empty())
     Commands.pop_back();
 
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(, "result": [)" + Commands +
       R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onCompletion(
-    TextDocumentPositionParams Params, StringRef ID, JSONOutput &Out) {
+void ClangdLSPServer::onCompletion(TextDocumentPositionParams Params,
+                                   StringRef ID, JSONOutput &Out) {
 
-  auto Items = LangServer.Server
+  auto Items = Server
                    .codeComplete(Params.textDocument.uri.file,
                                  Position{Params.position.line,
                                           Params.position.character})
                    .Value;
 
   std::string Completions;
   for (const auto &Item : Items) {
     Completions += CompletionItem::unparse(Item);
     Completions += ",";
   }
   if (!Completions.empty())
     Completions.pop_back();
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":[)" + Completions + R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onGoToDefinition(
-    TextDocumentPositionParams Params, StringRef ID, JSONOutput &Out) {
+void ClangdLSPServer::onGoToDefinition(TextDocumentPositionParams Params,
+                                       StringRef ID, JSONOutput &Out) {
 
-  auto Items = LangServer.Server
+  auto Items = Server
                    .findDefinitions(Params.textDocument.uri.file,
                                     Position{Params.position.line,
                                              Params.position.character})
                    .Value;
 
   std::string Locations;
   for (const auto &Item : Items) {
     Locations += Location::unparse(Item);
     Locations += ",";
   }
   if (!Locations.empty())
     Locations.pop_back();
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":[)" + Locations + R"(]})");
 }
 
-void ClangdLSPServer::LSPProtocolCallbacks::onSwitchSourceHeader(
-    TextDocumentIdentifier Params, StringRef ID, JSONOutput &Out) {
-  llvm::Optional<Path> Result =
-      LangServer.Server.switchSourceHeader(Params.uri.file);
+void ClangdLSPServer::onSwitchSourceHeader(TextDocumentIdentifier Params,
+                                           StringRef ID, JSONOutput &Out) {
+  llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
   std::string ResultUri;
   if (Result)
     ResultUri = URI::unparse(URI::fromFile(*Result));
   else
     ResultUri = "\"\"";
 
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":)" + ResultUri + R"(})");
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  bool SnippetCompletions,
                                  llvm::Optional<StringRef> ResourceDir)
-    : Out(Out), CDB(/*Logger=*/Out), DiagConsumer(*this),
-      Server(CDB, DiagConsumer, FSProvider, AsyncThreadsCount,
+    : Out(Out), CDB(/*Logger=*/Out),
+      Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
              SnippetCompletions, /*Logger=*/Out, ResourceDir) {}
 
 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));
-  registerCallbackHandlers(Dispatcher, Out, Callbacks);
+  registerCallbackHandlers(Dispatcher, Out, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, Dispatcher, IsDone);
 
   // Make sure IsDone is set to true after this method exits to ensure assertion
   // at the start of the method fires if it's ever executed again.
   IsDone = true;
 }
 
 std::vector<clang::tooling::Replacement>
 ClangdLSPServer::getFixIts(StringRef File, const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Lock(FixItsMutex);
   auto DiagToFixItsIter = FixItsMap.find(File);
   if (DiagToFixItsIter == FixItsMap.end())
     return {};
 
   const auto &DiagToFixItsMap = DiagToFixItsIter->second;
   auto FixItsIter = DiagToFixItsMap.find(D);
   if (FixItsIter == DiagToFixItsMap.end())
     return {};
 
   return FixItsIter->second;
 }
 
-void ClangdLSPServer::consumeDiagnostics(
-    PathRef File, std::vector<DiagWithFixIts> Diagnostics) {
+void ClangdLSPServer::onDiagnosticsReady(
+    PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) {
   std::string DiagnosticsJSON;
 
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
-  for (auto &DiagWithFixes : Diagnostics) {
+  for (auto &DiagWithFixes : Diagnostics.Value) {
     auto Diag = DiagWithFixes.Diag;
     DiagnosticsJSON +=
         R"({"range":)" + Range::unparse(Diag.range) +
         R"(,"severity":)" + std::to_string(Diag.severity) +
         R"(,"message":")" + llvm::yaml::escape(Diag.message) +
         R"("},)";
 
     // We convert to Replacements to become independent of the SourceManager.