[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:250
+  // Verify if path has value and is a valid path
+  if (Params.settings.compilationDatabasePath.hasValue()) {
+    CDB.setCompileCommandsDir(
----------------
Replace `Settings` instead of `Params.settings` here?
Or remove the `Settings` variable altogether.


================
Comment at: clangd/ClangdServer.cpp:579
+std::vector<std::future<void>> ClangdServer::reparseOpenedFiles() {
+
+  std::promise<void> DonePromise;
----------------
NIT: remote empty line


================
Comment at: clangd/ClangdServer.cpp:581
+  std::promise<void> DonePromise;
+  std::future<void> DoneFuture = DonePromise.get_future();
+  std::vector<std::future<void>> FutureVector;
----------------
`DonePromise` and `DoneFuture` are never used. Remove them?


================
Comment at: clangd/ClangdServer.cpp:587
+
+  for (auto FilePath : ActiveFilePaths)
+    FutureVector.push_back(forceReparse(FilePath));
----------------
Use "const auto&" or `StringRef` instead to avoid copies?


================
Comment at: clangd/ClangdServer.h:244
+  /// This method is normally called when the compilation database is changed.
+
+  std::vector<std::future<void>> reparseOpenedFiles();
----------------
NIT: remove this empty line


================
Comment at: clangd/GlobalCompilationDatabase.cpp:108
       Logger.log("Failed to find compilation database for " + Twine(File) +
-                 "in overriden directory " + CompileCommandsDir.getValue() +
+                 " in overriden directory " + CompileCommandsDir.getValue() +
                  "\n");
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > Accidental change?
> Twine(File) and "in overriden directory" did not have a space to separate otherwise.
Right. Sorry, my mistake.


================
Comment at: clangd/Protocol.h:294
+/// since the data received is described as 'any' type in LSP.
+
+struct ClangdConfigurationParamsChange {
----------------
NIT: remote empty line


================
Comment at: clangd/Protocol.h:296
+struct ClangdConfigurationParamsChange {
+
+  llvm::Optional<std::string> compilationDatabasePath;
----------------
NIT: remote empty line


================
Comment at: clangd/Protocol.h:303
+struct DidChangeConfigurationParams {
+
+  DidChangeConfigurationParams() {}
----------------
NIT: remove empty line


================
Comment at: clangd/Protocol.h:304
+
+  DidChangeConfigurationParams() {}
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings) {}
----------------
NIT: Use `= default` instead


================
Comment at: clangd/Protocol.h:305
+  DidChangeConfigurationParams() {}
+  DidChangeConfigurationParams(ClangdConfigurationParamsChange settings) {}
+
----------------
We need to initialize `settings` field with `settings` parameter.

Maybe even better to remove both constructors to align with other classes in this file?


================
Comment at: test/clangd/did-change-configuration.test:15
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":""}}
+#Failed to decode workspace/didChangeConfiguration request.
+#Incorrect mapping node
----------------
Do we want to add a `# CHECK:` for that output?


================
Comment at: test/clangd/did-change-configuration.test:33
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///compile_commands.json","languageId":"json","version":1,"text":"[\n{\n"directory":"/",\n"command":"/usr/bin/c++-DGTEST_HAS_RTTI=0-D_GNU_SOURCE-D__STDC_CONSTANT_MACROS-D__STDC_FORMAT_MACROS-D__STDC_LIMIT_MACROS-Ilib/Demangle-I../lib/Demangle-I/usr/include/libxml2-Iinclude-I../include-fPIC-fvisibility-inlines-hidden-Werror=date-time-std=c++11-Wall-W-Wno-unused-parameter-Wwrite-strings-Wcast-qual-Wno-missing-field-initializers-pedantic-Wno-long-long-Wno-maybe-uninitialized-Wdelete-non-virtual-dtor-Wno-comment-O0-g-fno-exceptions-fno-rtti-o/foo.c.o-c/foo.c",\n"file":"/foo.c"\n},"}}}
+
----------------
clangd won't see this file. `didOpen` only sets contents for diagnostics, not any other features.
You would rather want to add more `# RUN:` directives at the top of the file to create `compile_commands.json`, etc.

Writing it under root ('/') is obviously not an option. Lit tests allow you to use temporary paths, this is probably an approach you could take. See [[ https://llvm.org/docs/TestingGuide.html#substitutions | lit docs ]] for more details.


================
Comment at: test/clangd/initialize-params-invalid.test:2
+
 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 # It is absolutely vital that this file has CRLF line endings.
----------------
I am still seeing this newline


https://reviews.llvm.org/D39571