[PATCH] D40486: [clangd] Implemented logging using Context

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 23:50:57 PST 2017

sammccall added a comment.

In https://reviews.llvm.org/D40486#945193, @ilya-biryukov wrote:

> I'll update the implementation and place the `Context` as the first parameter everywhere instead.
>  Could you take a look at other changes in the patch while I'm at it?

Just pinging this - there's still places where Context is in a different position (at least in ClangdServer)

Comment at: clangd/ClangdServer.cpp:36
-  ~FulfillPromiseGuard() { Promise.set_value(); }
+  ~FulfillContextPromiseGuard() { Promise.set_value(std::move(Ctx)); }
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?

Comment at: clangd/ProtocolHandlers.h:32
-  using Ctx = RequestContext;
+  using Ctx = Context;
   virtual ~ProtocolCallbacks() = default;
We should use one name or the other. Fine to do this to keep the patch small, but leave a FIXME?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list