[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 16 09:25:43 PDT 2018
sammccall marked 9 inline comments as done.
sammccall added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:156
+void ClangdLSPServer::onExit(ExitParams &Params) {
+ // XXX handler should return true.
+}
----------------
ioeric wrote:
> ?
Fixed this comment.
I'm not totally happy with the shutdown mechanism, it's noisier and more magic than I'd like. May find something nicer to replace it with.
================
Comment at: clangd/JSONTransport.cpp:23
+ Code = L.Code;
+ return make_error<StringError>(L.Message, inconvertibleErrorCode());
+ }));
----------------
ioeric wrote:
> Is `E` guaranteed to be an `LSPError`? Do we need a handler for other errors?
This actually handles generic errors as well, Error just has a really confusing API.
Rewrote this to be more direct.
================
Comment at: clangd/Transport.h:50
+ virtual ~MessageHandler() = default;
+ virtual bool notify(llvm::StringRef Method, llvm::json::Value) = 0;
+ virtual bool call(llvm::StringRef Method, llvm::json::Value Params,
----------------
ioeric wrote:
> nit: maybe `handleNotify` to make the two directions more distinguishable? This can get a bit counter-intuitive in the call sites.
`onNotify` etc
================
Comment at: unittests/clangd/JSONTransportTests.cpp:106
+ auto Err = T->loop(E);
+ EXPECT_FALSE(bool(Err)) << llvm::toString(std::move(Err));
+
----------------
ioeric wrote:
> Would `operator<<` be used when the error is real (i.e. check passes)?
No, EXPECT_* are magic macros that only evaluate the right hand side of << if they fire.
`EXPECT_FALSE(expr)` expands to something like `if (expr) CreateErrorStream("expr")`, so the `<< blah` becomes part of the if statement.
(And the temporary stream returned from CreateErrorStream does the actual logging/mark-the-test-as-failed in its destructor).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53286
More information about the cfe-commits
mailing list