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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits