simark marked 7 inline comments as done.
simark added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:433
reparseOpenedFiles();
}
----------------
sammccall wrote:
> 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.)
Oh I have only done the change locally, I have not uploaded a new diff yet.
================
Comment at: clangd/ClangdLSPServer.h:90
void reparseOpenedFiles();
+ void applyConfiguration(const ClangdInitializationOptions &Settings);
void applyConfiguration(const ClangdConfigurationParamsChange &Settings);
----------------
sammccall wrote:
> 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?)
Indeed, since it's things that can only be set during initialization.
================
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional<std::string> compilationDatabasePath;
+};
----------------
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.
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
