kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks! can't wait for the unique_function sfinae fix :)
================
Comment at: clang-tools-extra/clangd/LSPBinder.h:185
+
+LSPBinder::UntypedOutgoingNotification inline LSPBinder::outgoingNotification(
+ llvm::StringLiteral Method) {
----------------
maybe put `inline` at the start of the declaration ? :D
================
Comment at: clang-tools-extra/clangd/LSPBinder.h:88
+ template <typename Request, typename Response>
+ void outgoingMethod(llvm::StringLiteral Method,
+ OutgoingMethod<Request, Response> &Handler) {
----------------
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!
================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:237
+ Value += X;
+ Changed(Value);
+ }
----------------
sammccall wrote:
> whoops, after rebasing against 40cc63ea6eec7874d3a358f9fa549ef2f6543512 this
> uncovered a bug - we're testing the public API from the wrong thread (server
> thread != main thread).
>
> So I've had to remove that test in this patch. We can test public APIs again
> in D96755.
yikes, i see what you mean now.. (i was searching for the failure in the
previous commit you mentioned)
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
