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