[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

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