From abeed660562dc03fe3e9514513d8e5609735d079 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 16 Oct 2018 15:55:03 +0000 Subject: [PATCH] Remove possibility to change compile database path at runtime Summary: 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. Subscribers: ilya-biryukov, ioeric, jkorous, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D53220 llvm-svn: 344614 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 27 +++++++------- clang-tools-extra/clangd/Protocol.cpp | 11 +++++- clang-tools-extra/clangd/Protocol.h | 11 ++++-- .../compile-commands-path-in-initialize.test | 11 +----- .../test/clangd/compile-commands-path.test | 42 ---------------------- 5 files changed, 33 insertions(+), 69 deletions(-) delete mode 100644 clang-tools-extra/test/clangd/compile-commands-path.test diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 02496ad..021c287 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -81,8 +81,16 @@ CompletionItemKindBitset defaultCompletionItemKinds() { } // namespace void ClangdLSPServer::onInitialize(InitializeParams &Params) { - if (Params.initializationOptions) - applyConfiguration(*Params.initializationOptions); + if (Params.initializationOptions) { + const ClangdInitializationOptions &Opts = *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::onHover(TextDocumentPositionParams &Params) { } void ClangdLSPServer::applyConfiguration( - const ClangdConfigurationParamsChange &Settings) { - // Compilation database change. - if (Settings.compilationDatabasePath.hasValue()) { - CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue()); - - reparseOpenedFiles(); - } - - // Update to the compilation database. - if (Settings.compilationDatabaseChanges) { - const auto &CompileCommandUpdates = *Settings.compilationDatabaseChanges; + const ClangdConfigurationParamsChange &Params) { + // Per-file update to the compilation database. + if (Params.compilationDatabaseChanges) { + const auto &CompileCommandUpdates = *Params.compilationDatabaseChanges; bool ShouldReparseOpenFiles = false; for (auto &Entry : CompileCommandUpdates) { /// The opened files need to be reparsed only when some existing diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp index 7a137eb..ced7cf2 100644 --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -665,10 +665,19 @@ bool fromJSON(const llvm::json::Value &Params, bool fromJSON(const json::Value &Params, ClangdConfigurationParamsChange &CCPC) { json::ObjectMapper O(Params); - return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) && + return O && O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges); } +bool fromJSON(const json::Value &Params, ClangdInitializationOptions &Opts) { + if (!fromJSON(Params, Opts.ParamsChange)) { + return false; + } + + json::ObjectMapper O(Params); + return O && O.map("compilationDatabasePath", Opts.compilationDatabasePath); +} + bool fromJSON(const json::Value &Params, ReferenceParams &R) { TextDocumentPositionParams &Base = R; return fromJSON(Params, Base); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index bd0973f..51eb707 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -411,8 +411,6 @@ bool fromJSON(const llvm::json::Value &, ClangdCompileCommand &); /// "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> @@ -420,7 +418,14 @@ struct ClangdConfigurationParamsChange { }; 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 diff --git a/clang-tools-extra/test/clangd/compile-commands-path-in-initialize.test b/clang-tools-extra/test/clangd/compile-commands-path-in-initialize.test index b34c595..ef7cd4f 100644 --- a/clang-tools-extra/test/clangd/compile-commands-path-in-initialize.test +++ b/clang-tools-extra/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", --- -{"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":10000,"method":"shutdown"} diff --git a/clang-tools-extra/test/clangd/compile-commands-path.test b/clang-tools-extra/test/clangd/compile-commands-path.test deleted file mode 100644 index f25d002..0000000 --- a/clang-tools-extra/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":10000,"method":"shutdown"} -- 2.7.4