[PATCH] D39571: [clangd] DidChangeConfiguration Notification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

It seems this patch is out of date, we need to merge it with the latests head.



================
Comment at: clangd/DraftStore.cpp:45
   std::lock_guard<std::mutex> Lock(Mutex);
+  assert(!File.empty());
 
----------------
Why do we need this assert? I don't see how empty file is worse than any other invalid value.


================
Comment at: clangd/DraftStore.cpp:55
   std::lock_guard<std::mutex> Lock(Mutex);
+  assert(!File.empty());
 
----------------
Why do we need this assert? I don't see how empty file is worse than any other invalid value.


================
Comment at: clangd/GlobalCompilationDatabase.h:65
+  void setCompileCommandsDir(Path P) {
+    CompileCommandsDir = P;
+    CompilationDatabases.clear();
----------------
We need to lock the Mutex here!


================
Comment at: clangd/Protocol.cpp:368
+json::Expr toJSON(const DidChangeConfigurationParams &CCP) {
+
+  return json::obj{
----------------
NIT: maybe remove this empty line?


================
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},"}}}
+
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > 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.
> Are there examples available on how to use this? I have to use a # RUN: to create a file and then use it's path in a workspace/didChangeConfiguration notification?
After thinking about it, I really don't see an easy way to test it currently. We just don't have the infrastructure in place for that.
I think it's fine to add a FIXME to the body of `ClangdLSPServer::onChangeConfiguration` that we need to test it and remove this test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571