sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
I think it'd be a good idea to separate out the on-initialization vs
dynamically-changing parameters more - I think they should probably be disjoint
in fact.
But we can discuss/implement that separately from this patch - the change
itself is really nice.
================
Comment at: clangd/ClangdLSPServer.cpp:433
reparseOpenedFiles();
}
----------------
sammccall wrote:
> This isn't needed, the compilation database can only be set during
> initialization.
It's still here... maybe forgot to upload a new diff?
(Just to be clear: I meant `reparseOpenFiles` doesn't need to be called, as
there are none.)
================
Comment at: clangd/ClangdLSPServer.h:90
void reparseOpenedFiles();
+ void applyConfiguration(const ClangdInitializationOptions &Settings);
void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
----------------
simark wrote:
> 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.
(In fact, this could also live directly in `onInitialize`, I think?)
================
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional<std::string> compilationDatabasePath;
+};
----------------
simark wrote:
> 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.
> Do you mean move it in the JSON, so it looks like this on the wire?
Well, since you asked... :-) I'm not going to push hard for it (this patch is
certainly a win already), but I do think that would be much clearer.
The current protocol has `InitializeParams` and `ClangdInitializationOptions`,
and it's not clear semantically what the distinction between them is.
With hindsight, I think something like this would be easier to follow:
```
// Clangd options that may be set at startup.
struct InitializeParams {
// Clangd extension: override the path used to load the CDB.
Optional<string> compilationDatabasePath;
// Provides initial configuration as if by workspace/updateConfiguration.
Optional<ClangdConfigurationParamsChange> initialConfiguration;
}
// Clangd options that may be set dynamically at runtime.
struct ClangdConfigurationParamsChange { ... }
```
though even here, the benefit from being able to inline the initial
configuration into the initalize message is unclear to me. The implementation
has to support dynamic updates in any case, so why not make use of that?
> We designed it so didChangeConfiguration and the initialization options would
> share the same structure
This makes sense, but if they're diverging, I'm not sure that keeping them
*mostly* the same brings more benefits than confusion.
------
That said, if you prefer to keep the JSON as it is, that's fine. (If we grow
more extensions, we may want to reorganize in future though?)
My main concern is the use of inheritance here, and how it provides a default
(configuration-change options can be provided at startup) that doesn't seem
obviously correct and is hard to opt out of.
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
