[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:51
+          "definitionProvider": true,
+          "configurationChangeProvider": true
         }})");
----------------
malaperle wrote:
> Are you sure the existing tests don't fail? usually when we change this, some tests have to be adjusted.
I'll verify this when I will have the time.


================
Comment at: clangd/ClangdServer.h:289
+  /// ChangedSettings
+  void changeConfiguration(std::map<std::string, std::string> ChangedSettings);
+
----------------
ilya-biryukov wrote:
> 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.
So if I understand correctly, we would have the most generic workspace/didChangeConfiguration handler located in ClangdLSPServer
 with a name perhaps similar to  changeConfiguration  that would pass valid settings to more specific functions inside ClangdServer ?


================
Comment at: clangd/Protocol.cpp:534
 
+llvm::Optional<DidChangeConfigurationParams>
+DidChangeConfigurationParams::parse(llvm::yaml::MappingNode *Params,
----------------
malaperle wrote:
> I think this needs a test, perhaps create did-change-configuration.test? It can also test the ClangdConfigurationParams::parse code
> If you do, I think it's a good idea to test a few failing cases:
> - "settings" is not a mapping node. You can test this with a scalar node like "settings": ""
> - Something else than "settings" is present, so that it goes through logIgnoredField
> - "settings" is not set at all. In this case, because it's mandatory in the protocol, return llvm::None. This can be checked after the loop after all key/values were read.
> 
> There are similar tests in execute-command.test if you'd like an example.
> And of course also add a "successful" case as well  :)
Yes, I was planning on adding a simple test in the coming days. I imagine this test will be greatly expanded on once more settings become available to change on the server side.


https://reviews.llvm.org/D39571