This is an archive of the discontinued LLVM Phabricator instance.

[clangd] textDocument/implementation (LSP layer)
ClosedPublic

Authored by usaxena95 on Nov 18 2020, 8:41 AM.

Diff Detail

Event Timeline

usaxena95 created this revision.Nov 18 2020, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 8:41 AM
usaxena95 requested review of this revision.Nov 18 2020, 8:41 AM

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?

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

nit: move it to line 605 (right after definitionProvider).

1443

nit: onGoToImplementation, move a line up to be closer with Go-to-{declaration/definition}.

clang-tools-extra/clangd/Protocol.h
1482

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.)

usaxena95 marked 3 inline comments as done.Nov 23 2020, 12:30 AM

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?

Yes. That was fixed last week. Rebased.

Addressed comments.

hokein accepted this revision.Nov 23 2020, 12:39 AM

thanks, looks good.

This revision is now accepted and ready to land.Nov 23 2020, 12:39 AM
This revision was landed with ongoing or failed builds.Nov 23 2020, 4:58 AM
This revision was automatically updated to reflect the committed changes.
kadircet added inline comments.Sep 14 2022, 1:05 AM
clang-tools-extra/clangd/ClangdLSPServer.cpp
1303

any particular reason for preferring declarations here that I am missing (in presence of a definition)?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 14 2022, 1:05 AM