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