[PATCH] D43227: [clangd] Make functions of ClangdServer callback-based

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D43227#1006584, @sammccall wrote:

> I wonder whether we want to introduce `using Callback<T...> = UniqueFunction<void(T...)>` for readability at some point...


I agree that's a good idea, will come up with a CL doing that.



================
Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+                 PCHs](Path File, decltype(Callback) Callback,
+                       llvm::Expected<InputsAndPreamble> IP) {
----------------
sammccall wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > nit: I'd probably use a different name than `Callback` for this parameter for clarity.
> > > I actually think we should keep it this way. It's a bit tricky to grasp, but it has two important advantages:
> > >   - Makes referencing `Callback` from outer scope impossible
> > >   - saves us from coming up with names for that superficial variable, i.e. should it be called `InnerCallback`, `Callback2`, `C` or something else?
> > > 
> > > Is it that too confusing or do you feel we can keep it?
> > > Makes referencing Callback from outer scope impossible. 
> > For lambdas, I think we should rely on captures for this instead of the parameter names.
> > >saves us from coming up with names for that superficial variable, i.e. should it be called InnerCallback, Callback2, C or something else?
> > I thought this is a common practice with llvm coding style :P I would vote for `C` :) 
> > 
> > > Is it that too confusing or do you feel we can keep it?
> > It is a bit confusing when I first looked at it, but nothing is *too* confusing as long as the code is correct ;) I just think it would make the code easier to read.
> I agree with you both :-)
> Conceptually, this *is* a capture - BindWithForward simulates move-capture that C++11 doesn't have native syntax for. So the same name seems appropriate to me - you want shadowing  for the same reasons you want captures to shadow.
I'll leave as is in this review, we have other callsites, doing the same thing, that we don't touch here. If we decide to change it, we should change all of them in one go.
I'm not opposed to the change, but like the current style a bit more.


================
Comment at: unittests/clangd/SyncAPI.cpp:69
+  llvm::Expected<Tagged<SignatureHelp>> Result = Tagged<SignatureHelp>();
+  (void)(bool)Result; // Expected has to be checked.
+  Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
----------------
sammccall wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > I'd expect this to be checked by callers. Would `return std::move(Result);` work?
> > > The reason why we need it is because `capture(Result)` writes return value of the callback to the `Result` variable. But we have to first check the default-constructed value that was there **before** calling `Server.signatureHelp`
> > I think we should probably fix the behavior of `capture` since this changes the behavior of `llvm::Expected` in user code - the check is there to make sure users always check the status. But here we always do this for users.
> +1
> 
> I suggested `capture(T&)` was the right API because I thought it'd be used directly by test code (but we wrap it here) and because I thought T would be default-constructible without problems (but it's not).
> 
> I'd suggest changing to `capture(Optional<T>&)` instead, so the caller can just create an empty optional. That makes the callsites here slightly less obscure.
The users still have to check the returned value. We only check the **default-constructed** value. After `capture()` runs `Result` is reassigned and now has to be checked again (and this should be done by the users).
Replaced with `capture(Optional<T>&)` to make it absolutely clear we're doing the right thing there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227