sammccall accepted this revision.
sammccall added a comment.
Still LG!
================
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional<std::string> compilationDatabasePath;
+};
----------------
simark wrote:
> sammccall wrote:
> > 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.
> > The current protocol has InitializeParams and ClangdInitializationOptions,
> > and it's not clear semantically what the distinction between them is.
>
> `InitializeParams` is the type for the parameters of the `initialize`
> request. In it, there is this field:
>
> ```
> /**
> * User provided initialization options.
> */
> initializationOptions?: any;
> ```
>
> which is made to pass such language-server-specific options. Since it's of
> the `any` type, we gave it a name, `ClangdInitializationOptions`.
>
> > With hindsight, I think something like this would be easier to follow:
> >
> > [snippet]
>
> I don't really understand why putting fields as extra fields in
> `InitializeParams` would be any easier than putting them in the object that
> exists specifically for that purpose.
>
> > 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?
>
> I'd say, to avoid having the server start some work (like indexing some files
> using a certain set of flags) that will be invalidated when it receives some
> config changes moments later.
>
> > This makes sense, but if they're diverging, I'm not sure that keeping them
> > *mostly* the same brings more benefits than confusion.
>
> I think you are exaggerating the confusion. It is pretty straightforward:
> everything you can change during execution, you can also specify at
> initialize time.
> which is made to pass such language-server-specific options. Since it's of
> the any type, we gave it a name, `ClangdInitializationOptions`.
Fair enough. I personally think `initializationOptions` in LSP is not useful as
specified for various reasons, and would rather avoid it (it's optional), but
following the spirit of the spec makes sense too.
> I'd say, to avoid having the server start some work (like indexing some files
> using a certain set of flags) that will be invalidated when it receives some
> config changes moments later.
Doing this to enable optimizations definitely makes sense.
It does seem premature when we don't have any such optimization: it puts more
burden on clients by giving them multiple equivalent ways to do things, and
hinting that they're not quite equivalent.
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
