[PATCH] D39571: [clangd] DidChangeConfiguration Notification
William Enright via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 8 14:15:36 PST 2017
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
More information about the cfe-commits
mailing list