[PATCH] D96717: [clangd] Bind outgoing calls through LSPBinder too. NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

mostly LG. some comments around the way we set up outgoinmethod stubs though.



================
Comment at: clang-tools-extra/clangd/LSPBinder.h:88
+  template <typename Request, typename Response>
+  void outgoingMethod(llvm::StringLiteral Method,
+                      OutgoingMethod<Request, Response> &Handler) {
----------------
well this one filling an out-parameter definitely tripped me over while reading the code a couple times, and once more on the tests.

what about making this return the handler instead? E.g. `Edit = Bind.outgoingMethod<EditParams, EditResult>("edit")`, I know it is ugly that we duplicate template params on declaration + here now. Maybe we can get away by making `OutgoingMethod` a class with a call operator and a `RawOutgoing *Out` member that can be bound later on ? e.g:

```
template <StringLiteral Method, typename P, typename R>
class OutgoingMethod {
  RawOutgoing *Out = nullptr;
public:
  void operator()(const P& Params, Callback<R> CB) {
    assert(Out && "Method haven't bound");
    Out->callMethod(Method, JSON(Params), ....);
  }
};
```

then we either make LSPBinder a friend of OutgoingMethod and set Out through it, or the other way around and have a `OutgoingMethod::bindOut(LSPBinder&)` ?

not sure if it is any better though, as we still construct a somewhat invalid object :/


================
Comment at: clang-tools-extra/clangd/LSPBinder.h:90
+                      OutgoingMethod<Request, Response> &Handler) {
+    Handler = [Method, Out(&Out)](Request R, Callback<Response> Reply) {
+      Out->callMethod(
----------------
nit: maybe move bodies for these functions out-of-line too?


================
Comment at: clang-tools-extra/clangd/LSPBinder.h:93
+          Method, JSON(R),
+          // FIXME: why keep ctx alive but not restore it for the callback?
+          [Reply(std::move(Reply)), Ctx(Context::current().clone()),
----------------
haha feels like confusing `Context` and `WithContext` issue i talked about yesterday :D


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96717/new/

https://reviews.llvm.org/D96717