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