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