[PATCH] D91721: [clangd] textDocument/implementation (LSP layer)

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

just nits.

Looks like the patch is based on the old revision (pre-merging tests are failing), I assume you have fixed the failure tests last week?



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:615
             {"workspaceSymbolProvider", true},
+            {"implementationProvider", true},
             {"referencesProvider", true},
----------------
nit: move it to line 605 (right after `definitionProvider`).


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1443
   MsgHandler->bind("textDocument/references", &ClangdLSPServer::onReference);
+  MsgHandler->bind("textDocument/implementation", &ClangdLSPServer::onImplementation);
   MsgHandler->bind("textDocument/switchSourceHeader", &ClangdLSPServer::onSwitchSourceHeader);
----------------
nit: `onGoToImplementation`, move a line up to be closer with Go-to-{declaration/definition}.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1482
 
+struct ImplementationParams : public TextDocumentPositionParams {};
+bool fromJSON(const llvm::json::Value &, ImplementationParams &,
----------------
nit: since this is just a mirror of `TextDocumentPositionParams`, I'd use `TextDocumentPositionParams` directly (looks like this is a pattern of other features, go-to-definition etc.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91721/new/

https://reviews.llvm.org/D91721