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

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.

lg



================
Comment at: clangd/ClangdServer.cpp:209
+  auto Action = [Contents, Pos, TaggedFS,
+                 PCHs](Path File, decltype(Callback) Callback,
+                       llvm::Expected<InputsAndPreamble> IP) {
----------------
ilya-biryukov wrote:
> 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.
Feel free to land without changing this; this is a nit anyway ;)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43227