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