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:
> 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.
================
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()),
----------------
kadircet wrote:
> haha feels like confusing `Context` and `WithContext` issue i talked about
> yesterday :D
Exactly. I'm fairly sure we need a WithContext here but don't want to risk
changing it at the same time.
================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:237
+ Value += X;
+ Changed(Value);
+ }
----------------
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.
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
