[PATCH] D42499: [clangd] Modify the Span API so that Spans propagate with contexts.

[Ilya Biryukov via Phabricator via cfe-commits]

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