simark marked an inline comment as done.
simark added inline comments.
================
Comment at: clangd/ClangdLSPServer.h:90
void reparseOpenedFiles();
+ void applyConfiguration(const ClangdInitializationOptions &Settings);
void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
----------------
sammccall wrote:
> Prefer a different name for this function - an overload set should have
> similar semantics and these are quite different (pseudo-constructor vs
> mutation allowed at any time).
Ok, I'll think about a better name.
================
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional<std::string> compilationDatabasePath;
+};
----------------
sammccall wrote:
> Can we just move this to InitializeParams as a clangd extension?
> Doing tricks with inheritance here makes the protocol harder to understand.
> Can we just move this to InitializeParams as a clangd extension?
Do you mean move it in the JSON, so it looks like this on the wire?
```
{
"method": "initialize",
"params": {
"compilationDatabasePath": "<path>",
...
}
}
```
instead of
```
{
"method": "initialize",
"params": {
"initializationOptions": {
"compilationDatabasePath": "<path>"
},
...
}
}
```
?
I think `initializationOptions` is a good place for this to be, I wouldn't
change that.. If you don't like the inheritance, we can just get rid of it in
our code and have two separate versions of the deserializing code. We designed
it so `didChangeConfiguration` and the initialization options would share the
same structure, but it doesn't have to stay that way.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53220
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
