sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG, suggest a tweak to capture() though.
I wonder whether we want to introduce `using Callback<T...> =
UniqueFunction<void(T...)>` for readability at some point...
================
Comment at: clangd/ClangdServer.cpp:209
+ auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+ llvm::Expected<InputsAndPreamble> IP) {
----------------
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.
================
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);
----------------
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43227
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
