[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.

I don't think we should pass the very general configuration map to `ClangdServer`. Especially given that we can easily update `DirectoryBaseCompilationDatabase` instance in `ClangdLSPServer` itself.
What are your thoughts on this?



================
Comment at: clangd/ClangdLSPServer.cpp:199
+    Ctx C, DidChangeConfigurationParams &Params) {
+  std::map<std::string, std::string> SettingsMap;
+  SettingsMap.insert(std::pair<std::string, std::string>(
----------------
malaperle wrote:
> I'm thinking, maybe it's better not to have the map and just pass along the ClangdConfigurationParams to the server (instead of the map). I think ClangdConfigurationParams is more useful as a structure than a "flat" map with all the keys being at the same level. In ClangdConfigurationParams, we'll be able to add sub-configurations sections (index, code-completion, more?) which is well suited to reflect the JSON format.
> 
> Unless perhaps you had another use case for the map that I'm not thinking about?
I don't think `ClangdServer` should handle changes to compilation database path at all.
It accepts `GlobalCompilationDatabase` as a parameter and does not own it, so `ClangdLSPServer` can mutate it properly.


================
Comment at: clangd/ClangdServer.h:288
+  /// Modify configuration settings based on what is contained inside
+  /// ChangedSettings
+  void changeConfiguration(std::map<std::string, std::string> ChangedSettings);
----------------
NIT: Add full stop at the end of comments.


================
Comment at: clangd/ClangdServer.h:289
+  /// ChangedSettings
+  void changeConfiguration(std::map<std::string, std::string> ChangedSettings);
+
----------------
This function is way too general for `ClangdServer`'s interface, can we have more fine-grained settings here (i.e. `setCompletionParameters` etc?) and handle the "general" case in `ClangdLSPServer` (i.e., unknown setting names, invalid setting parameters, json parsing, etc.)?

I suggest we remove this function altogether.


https://reviews.llvm.org/D39571