[PATCH] D96717: [clangd] Bind outgoing calls through LSPBinder too. NFC
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 16 13:51:48 PST 2021
sammccall marked 2 inline comments as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/LSPBinder.h:88
+ template <typename Request, typename Response>
+ void outgoingMethod(llvm::StringLiteral Method,
+ OutgoingMethod<Request, Response> &Handler) {
----------------
kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > 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 :/
> > > well this one filling an out-parameter definitely tripped me over while reading the code a couple times, and once more on the tests.
> >
> > Yup. My feeling is the wins are:
> > - avoid writing the types over and over (we can't really avoid it on the instance variable)
> > - resembles syntax for binding incoming things
> >
> > And the main loss is definitely that we're not using assignment syntax for assignment.
> >
> > > what about making this return the handler instead? E.g. Edit = Bind.outgoingMethod<EditParams, EditResult>("edit")
> >
> > This is a reasonable alternative, as you say the duplication is the downside.
> >
> > > Maybe we can get away by making OutgoingMethod a class with a call operator
> >
> > Yeah, I don't really understand what this achieves - the usage looks the same, just now the logical assignment doesn't use assignment syntax *or* assignment semantics :-)
> >
> > Other ideas I had:
> > - the object is untyped, and the call operator is templated/typed - no type-safety at callsite, yuck
> > - `outgoingMethod` returns an untyped proxy object with a template conversion operator to unique_function<Req, Rsp>. This gives us assignment syntax... and horrible compiler errors on mismatch.
> >
> > I'm going to try the latter out to see how bad it is.
> thanks! this looks a whole lot better!
Glad you like it... personally I felt a little ill after writing it :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96717/new/
https://reviews.llvm.org/D96717
More information about the cfe-commits
mailing list