[PATCH] D53286: [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction.

[Sam McCall via Phabricator via cfe-commits]

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