ilya-biryukov added a comment.
I think all the comments are addressed now and this change is ready for another
round of review.
Let me know if I missed something.
================
Comment at: clangd/ClangdServer.cpp:36
- ~FulfillPromiseGuard() { Promise.set_value(); }
+ ~FulfillContextPromiseGuard() { Promise.set_value(std::move(Ctx)); }
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > Yikes, I can see how we got here, but we really don't get to move out of
> > > something we received an lvalue-reference to...
> > >
> > > I think a safer idiom here is turning a whole lambda into a destructor:
> > >
> > > auto Fulfil = MakeDestructor([&]{
> > > DonePromise.set_value(std::move(Ctx));
> > > });
> > >
> > > I'm not sure if LLVM has a shared utility for this, I've seen it in other
> > > codebases. Either way we could just define it here.
> > >
> > > (Or we could copy the context to avoid the awkwardness)
> > Thanks for suggestion. I totally agree, callbacks on scope exit are better.
> >
> > I couldn't find an utility like that in LLVM, but I think it's a useful
> > one. I'll add this locally to this file, but maybe we should make this
> > helper public and put it into clangd?
> > What are you thoughts on that?
> Sounds good. I'm wary of adding lots of tiny utility headers - maybe if we
> squint it could go in Function.h?
Then Function.h it is :-)
================
Comment at: clangd/ProtocolHandlers.h:32
public:
- using Ctx = RequestContext;
+ using Ctx = Context;
virtual ~ProtocolCallbacks() = default;
----------------
sammccall wrote:
> We should use one name or the other. Fine to do this to keep the patch small,
> but leave a FIXME?
Added a FIXME, will send a follow-up change that removes it after this change
lands.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
