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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
