[PATCH] D53220: Remove possibility to change compile database path at runtime
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344614: Remove possibility to change compile database path
at runtime (authored by simark, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53220?vs=169828=169833#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53220
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -665,10 +665,19 @@
bool fromJSON(const json::Value ,
ClangdConfigurationParamsChange ) {
json::ObjectMapper O(Params);
- return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+ return O &&
O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
}
+bool fromJSON(const json::Value , ClangdInitializationOptions ) {
+ if (!fromJSON(Params, Opts.ParamsChange)) {
+return false;
+ }
+
+ json::ObjectMapper O(Params);
+ return O && O.map("compilationDatabasePath", Opts.compilationDatabasePath);
+}
+
bool fromJSON(const json::Value , ReferenceParams ) {
TextDocumentPositionParams = R;
return fromJSON(Params, Base);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -81,8 +81,16 @@
} // namespace
void ClangdLSPServer::onInitialize(InitializeParams ) {
- if (Params.initializationOptions)
-applyConfiguration(*Params.initializationOptions);
+ if (Params.initializationOptions) {
+const ClangdInitializationOptions = *Params.initializationOptions;
+
+// Explicit compilation database path.
+if (Opts.compilationDatabasePath.hasValue()) {
+ CDB.setCompileCommandsDir(Opts.compilationDatabasePath.getValue());
+}
+
+applyConfiguration(Opts.ParamsChange);
+ }
if (Params.rootUri && *Params.rootUri)
Server->setRootPath(Params.rootUri->file());
@@ -425,17 +433,10 @@
}
void ClangdLSPServer::applyConfiguration(
-const ClangdConfigurationParamsChange ) {
- // Compilation database change.
- if (Settings.compilationDatabasePath.hasValue()) {
-CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue());
-
-reparseOpenedFiles();
- }
-
- // Update to the compilation database.
- if (Settings.compilationDatabaseChanges) {
-const auto = *Settings.compilationDatabaseChanges;
+const ClangdConfigurationParamsChange ) {
+ // Per-file update to the compilation database.
+ if (Params.compilationDatabaseChanges) {
+const auto = *Params.compilationDatabaseChanges;
bool ShouldReparseOpenFiles = false;
for (auto : CompileCommandUpdates) {
/// The opened files need to be reparsed only when some existing
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -411,16 +411,21 @@
/// "initialize" request and for the "workspace/didChangeConfiguration"
/// notification since the data received is described as 'any' type in LSP.
struct ClangdConfigurationParamsChange {
- llvm::Optional compilationDatabasePath;
-
// The changes that happened to the compilation database.
// The key of the map is a file name.
llvm::Optional>
compilationDatabaseChanges;
};
bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+struct ClangdInitializationOptions {
+ // What we can change throught the didChangeConfiguration request, we can
+ // also set through the initialize request (initializationOptions field).
+ ClangdConfigurationParamsChange ParamsChange;
+
+ llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdInitializationOptions &);
struct InitializeParams {
/// The process Id of the parent process that started
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s
[PATCH] D53220: Remove possibility to change compile database path at runtime
sammccall accepted this revision.
sammccall added a comment.
Still LG!
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional 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": "",
> > > ...
> > > }
> > > }
> > > ```
> > >
> > > instead of
> > >
> > > ```
> > > {
> > > "method": "initialize",
> > > "params": {
> > > "initializationOptions": {
> > > "compilationDatabasePath": ""
> > > },
> > > ...
> > > }
> > > }
> > > ```
> > >
> > > ?
> > >
> > > 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 compilationDatabasePath;
> > // Provides initial configuration as if by workspace/updateConfiguration.
> > Optional 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,
[PATCH] D53220: Remove possibility to change compile database path at runtime
simark updated this revision to Diff 169828.
simark marked 3 inline comments as done.
simark added a comment.
Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53220
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ /dev/null
@@ -1,42 +0,0 @@
-# Test that we can switch between configuration/build using the
-# workspace/didChangeConfiguration notification.
-
-# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
-# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
-# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
-
-# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
-
-# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
-# (with the extra slash in the front), so we add it here.
-# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
-
-# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is not defined",
-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is one",
-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is two",
-{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
@@ -18,18 +16,11 @@
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
# CHECK: "method": "textDocument/publishDiagnostics",
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
# CHECK-NEXT: {
# CHECK-NEXT: "message": "MACRO is one",
---
[PATCH] D53220: Remove possibility to change compile database path at runtime
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 );
void applyConfiguration(const ClangdConfigurationParamsChange );
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 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": "",
> > ...
> > }
> > }
> > ```
> >
> > instead of
> >
> > ```
> > {
> > "method": "initialize",
> > "params": {
> > "initializationOptions": {
> > "compilationDatabasePath": ""
> > },
> > ...
> > }
> > }
> > ```
> >
> > ?
> >
> > 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 compilationDatabasePath;
> // Provides initial configuration as if by workspace/updateConfiguration.
> Optional 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
[PATCH] D53220: Remove possibility to change compile database path at runtime
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 );
void applyConfiguration(const ClangdConfigurationParamsChange );
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 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": "",
> ...
> }
> }
> ```
>
> instead of
>
> ```
> {
> "method": "initialize",
> "params": {
> "initializationOptions": {
> "compilationDatabasePath": ""
> },
> ...
> }
> }
> ```
>
> ?
>
> 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 compilationDatabasePath;
// Provides initial configuration as if by workspace/updateConfiguration.
Optional 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
[PATCH] D53220: Remove possibility to change compile database path at runtime
simark marked an inline comment as done.
simark added inline comments.
Comment at: clangd/ClangdLSPServer.h:90
void reparseOpenedFiles();
+ void applyConfiguration(const ClangdInitializationOptions );
void applyConfiguration(const ClangdConfigurationParamsChange );
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 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": "",
...
}
}
```
instead of
```
{
"method": "initialize",
"params": {
"initializationOptions": {
"compilationDatabasePath": ""
},
...
}
}
```
?
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
[PATCH] D53220: Remove possibility to change compile database path at runtime
sammccall added a comment.
Thanks for cleaning this up!
Comment at: clangd/ClangdLSPServer.cpp:433
reparseOpenedFiles();
}
This isn't needed, the compilation database can only be set during
initialization.
Comment at: clangd/ClangdLSPServer.h:90
void reparseOpenedFiles();
+ void applyConfiguration(const ClangdInitializationOptions );
void applyConfiguration(const ClangdConfigurationParamsChange );
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).
Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+ llvm::Optional compilationDatabasePath;
+};
Can we just move this to InitializeParams as a clangd extension?
Doing tricks with inheritance here makes the protocol harder to understand.
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
[PATCH] D53220: Remove possibility to change compile database path at runtime
simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric,
ilya-biryukov.
This patch removes the possibility to change the compilation database
path at runtime using the didChangeConfiguration request. Instead, it
is suggested to use the setting on the initialize request, and clangd
whenever the user wants to use a different build configuration.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53220
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/Protocol.cpp
clangd/Protocol.h
test/clangd/compile-commands-path-in-initialize.test
test/clangd/compile-commands-path.test
Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ /dev/null
@@ -1,42 +0,0 @@
-# Test that we can switch between configuration/build using the
-# workspace/didChangeConfiguration notification.
-
-# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
-# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
-# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
-
-# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
-
-# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
-# (with the extra slash in the front), so we add it here.
-# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
-
-# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is not defined",
-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is one",
-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "MACRO is two",
-{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
@@ -18,18 +16,11 @@
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#else\n#pragma message (\"woops\")\n#endif\nint main()
