This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Refactor ProtocolHandlers to decouple them from ClangdLSPServer
ClosedPublic

Authored by ilya-biryukov on May 15 2017, 8:02 AM.

Details

Summary

A refactoring to decouple ProtocolHandlers and Language Server input parsing
loop from the ClangdLSPServer.
The input parsing was extracted from main to a function(runLanguageServerLoop).
ProtocolHandlers now provide an interface to handle various LSP methods,
this interface is used by ClangdLSPServer.
Methods for code formatting were moved from ProtocolHandlers to ClangdServer.
ClangdLSPServer now provides a cleaner interface that only runs Language Server
input loop.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.May 15 2017, 8:02 AM
ilya-biryukov added a project: Restricted Project.May 15 2017, 8:03 AM
ilya-biryukov added a subscriber: cfe-commits.

Fixed a typo in a comment

krasimir added inline comments.May 16 2017, 2:55 AM
clangd/ClangdLSPServer.cpp
206 ↗(On Diff #99009)

nit: .

211 ↗(On Diff #99009)

nit: .

clangd/ClangdLSPServer.h
49 ↗(On Diff #99009)

This deserves a comment.

33 ↗(On Diff #99008)

It seems strange to have the In here and the Out in the constructor.

krasimir edited edge metadata.May 16 2017, 5:25 AM

nit: rename this patch title to start with [clangd]

krasimir added inline comments.May 16 2017, 5:29 AM
clangd/ClangdLSPServer.cpp
20 ↗(On Diff #99009)

Hm, this is a bit too generic for my taste. Is it ever used generically?

ilya-biryukov marked 3 inline comments as done.

Addressed review comments

ilya-biryukov retitled this revision from [ClangD] Refactor ProtocolHandlers to decouple them from ClangdLSPServer to [clangd] Refactor ProtocolHandlers to decouple them from ClangdLSPServer.May 16 2017, 5:38 AM
ilya-biryukov added inline comments.
clangd/ClangdLSPServer.cpp
20 ↗(On Diff #99009)

Nope, it's now only used with one type, specialized it to it.

clangd/ClangdLSPServer.h
33 ↗(On Diff #99008)

Didn't want to store unnecessary fields in the class, but we do need JSONOutput &Out as a member, since there's consumeDiagnostics which uses it.
Would you prefer std::istream &In to be moved into a constructor parameter and stored as a field?

krasimir accepted this revision.May 16 2017, 6:27 AM

Looks good!

clangd/ClangdLSPServer.h
33 ↗(On Diff #99008)

I get it now. It's OK as it is now!

This revision is now accepted and ready to land.May 16 2017, 6:27 AM
ilya-biryukov closed this revision.May 16 2017, 7:53 AM
This revision was automatically updated to reflect the committed changes.