[PATCH] D133339: [clangd] Isolate logic for setting LSPServer options

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I like the change to initialize() a lot.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:64
+
+    /// Options used for diagnostics.
+    ClangdDiagnosticOptions DiagOpts;
----------------
I don't really like making these options part of this struct.

Currently this struct is largely used to send data from main->ClangdLSPServer.
After this patch, half of the fields are for that and half of them are scratch space for ClangdLSPServer to use itself. Worse, they *look* like things that can/should be set beforehand, but will in fact the passed values are ignored.
(Admittedly, some of the *inherited* fields are used in the same way, sigh).

Additionally, these options are publicly exposed when they don't need to be - these are internals leaking into API without a use case.

---

Having updateServerOptions be a hidden helper rather than a member is nice, but I think not as important as the actual public interface.

Grouping the options is also nice. Having the members be consecutive seems enough to me (they're almost grouped: background queue state is in the wrong place). Adding a second struct also seems like an option though?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133339