This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Extract ClangdServer::Options struct.
ClosedPublic

Authored by sammccall on Mar 5 2018, 5:17 AM.

Details

Summary

This subsumes most of the params to ClangdServer and ClangdLSPServer.
Adjacent changes:

  • tests use a consistent set of options, except when testing specific options
  • tests that previously used synchronous mode for convenience no longer do
  • added a runAddDocument helper to SyncAPIs to mitigate the extra code
  • rearranged main a bit to follow the structure of the options

Event Timeline

sammccall created this revision.Mar 5 2018, 5:17 AM
ilya-biryukov added inline comments.Mar 5 2018, 6:19 AM
clangd/ClangdServer.h
94

Maybe return a reference instead of a pointer?

107

NIT: remove empty comment?

136

Could we keep FSProvider a separate parameter?

  • Having it as a reference clearly states that it can't be null on the type level.
  • It is a C++ extension point, rather than a configuration parameter. I'd vouch for not mixing those together in the same struct.

We could group GlobalCompilationDatabase, DiagnosticsConsumer, FSProvider into a separate struct if the number of parameters is a concern. I'd put StaticIndex there as well, even though that would mean two index-related options are separated.

sammccall updated this revision to Diff 136991.Mar 5 2018, 6:46 AM
sammccall marked 3 inline comments as done.

address review comments

ilya-biryukov accepted this revision.Mar 5 2018, 7:17 AM
This revision is now accepted and ready to land.Mar 5 2018, 7:17 AM
This revision was automatically updated to reflect the committed changes.
jkorous-apple added inline comments.Mar 8 2018, 4:10 AM
clang-tools-extra/trunk/clangd/ClangdServer.h
131 ↗(On Diff #137025)

Shouldn't this be better implemented as a utility function in tests?

Also what is the purpose of making global function in header file static?