ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM. And sorry for confusion with https://reviews.llvm.org/D42524, if any.



================
Comment at: clangd/Function.h:147
 
-  ScopeExitGuard(Func F) : F(std::move(F)) {}
+  ScopeExitGuard(Func F) : F(llvm::make_unique<Func>(std::move(F))) {}
   ~ScopeExitGuard() {
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Why do we need `unique_ptr` instead of `Optional` here?
> ScopeExitGuard was broken by the recent Optional changes (when Func is 
> trivially copyable).
> This is a minimal workaround, let's discuss on the other patch.
LG, thanks. Wasn't aware of the breakage when writing the comment.


================
Comment at: clangd/JSONRPCDispatcher.cpp:31
+  RequestArgs(json::obj *Args) : Args(Args) {}
+  std::mutex Mu;
+  json::obj *Args;
----------------
We could make the fields private and expose only the function to modify args 
under lock (similar to `FillRequestInfo`, i.e. the last two line of it).
That would make the class safer to use. But that's work and the class itself is 
an implementation detail, so we could also leave it as is.


================
Comment at: clangd/Trace.cpp:138
+                  Args)
+            : Ctx.derive(kSpanArgs, nullptr)) {}
 
----------------
Would be nice for `Span`s to have zero cost when no tracer is active. We could 
probably achieve that be not storing the `kSpanArgs` when `T` is null. WDYT?
That's really not a big deal, so I don't think we should block the patch on 
this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42499



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to