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