[PATCH] D38939: [clangd] Handle exit notification (proper shutdown)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:59
 void ClangdLSPServer::onShutdown(Ctx C, ShutdownParams &Params) {
-  IsDone = true;
+  // Before we reply, we could serialize the preambles to disk. For now, let's
+  // just say we're ready to exit.
----------------
The comment is a bit misleading. I'd simply say we don't do need to do anything on `shutdown` at this point.

offtopic:
Preambles is not something we'd want to serialise (they are rather fast to build, and we only need them to make code completion fast), properly saving state of various indexes when we'll have them is something we'll definitely need to do on shutdown, though.


================
Comment at: clangd/tool/ClangdMain.cpp:117
                             ResourceDirRef, CompileCommandsDirPath);
-  LSPServer.run(std::cin);
+  return LSPServer.run(std::cin) ? 0 : 1;
 }
----------------
Maybe add a `const NoShutdownRequestErrorCode = 1;` and  use it here instead of `1`?
Or add a comment on why we exit with error code. (It's not entirely obvious to someone who hasn't read this part of LSP spec)


================
Comment at: test/clangd/protocol.test:1
-# RUN: clangd -run-synchronously < %s | FileCheck %s
-# RUN: clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+# RUN: not clangd -run-synchronously < %s | FileCheck %s
+# RUN: not clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
----------------
Maybe add a comment why we use `not` here?
It's not obvious for someone who hasn't read the whole test file. 


https://reviews.llvm.org/D38939