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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
