This revision was automatically updated to reflect the committed changes.
Closed by commit rL316564: [clangd] Handle exit notification (proper shutdown)
(authored by ibiryukov).
Changed prior to commit:
https://reviews.llvm.org/D38939?vs=119941&id=120210#toc
Repository:
rL LLVM
https://revi
ilya-biryukov added a comment.
In https://reviews.llvm.org/D38939#904318, @rwols wrote:
> I'll have to think about if I want that responsibility!
While you're at it, I'll submit this patch for you. But I'd definitely
encourage you to get commit access. You've been contributing a bunch of usefu
malaperle accepted this revision.
malaperle added a comment.
Nice! I had a fix locally for the same purpose but this is much better!
https://reviews.llvm.org/D38939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bi
rwols marked 3 inline comments as done.
rwols added a comment.
> you have commit access now, right?
I'll have to think about if I want that responsibility!
https://reviews.llvm.org/D38939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http
rwols updated this revision to Diff 119941.
rwols added a comment.
- Clarify why we invert the tests in protocol.test,
- Use a variable name to clarify why we exit with status code 1 (when run()
returns true),
- Reword misleading comment in onShutdown method.
https://reviews.llvm.org/D38939
Fi
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Oh, and other than those NITs, LGTM.
@rwols, you have commit access now, right?)
https://reviews.llvm.org/D38939
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lis
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.
rwols added a comment.
> So I'm not totally convinced that treating an abrupt exit or EOF as an error
> is worth the hassle
Agreed, at least from the POV of a client clangd shuts down gracefully now.
https://reviews.llvm.org/D38939
___
cfe-commits
rwols updated this revision to Diff 119370.
rwols added a comment.
- Add test shutdown-with-exit.test
- Add test shutdown-without-exit.test
- Make protocol.test pass by inverting exit code ("# RUN: not ...")
https://reviews.llvm.org/D38939
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPS
sammccall accepted this revision.
sammccall added a comment.
So I'm not totally convinced that treating an abrupt exit or EOF as an error is
worth the hassle (others might disagree), but it's certainly reasonable.
Could you add a test for the new behavior?
something like
RUN: not clangd -run-syn
rwols added a comment.
I couldn't get the protocol.test to succeed; `clangd` returns 1 now and that
trips up `lit`. I think I have to use `XFAIL` from `lit` somehow but didn't
figure it out.
https://reviews.llvm.org/D38939
___
cfe-commits mailing
rwols updated this revision to Diff 119198.
rwols added a comment.
Return 0 if shutdown was received before exit, otherwise return 1
The protocol.test test fails now.
https://reviews.llvm.org/D38939
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/Protocol.h
clangd/Pro
rwols added inline comments.
Comment at: test/clangd/authority-less-uri.test:33
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"jsonrpc":"2.0","id":3,"result":null}
+Content-Length: 33
sammccall wrote:
> Having the shutdown/exit boilerplate in every tes
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: test/clangd/authority-less-uri.test:33
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"jsonrpc":"2.0","id":3,"result":null}
+Content-Length: 3
rwols created this revision.
This changes the onShutdown handler to do essentially nothing (for now), and
instead exits the runloop when we receive the exit notification from the client.
Some clients may wait on the reply from the shutdown request before sending an
exit notification. If we exit t
15 matches
Mail list logo
