[PATCH] D40132: [clangd] Tracing improvements

[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



================
Comment at: clangd/JSONRPCDispatcher.h:78
   llvm::Optional<json::Expr> ID;
+  std::unique_ptr<trace::Span> Tracer;
 };
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Why do we need `unique_ptr`? Are `Span`s non-movable?
> Spans aren't movable, they have an explicitly declared destructor. (The copy constructor is only deprecated by this condition, but Span has a unique_ptr member).
> 
> We could make them movable, though it's not absolutely trivial (we need an extra bool to indicate that this is moved-from and the destructor should be a no-op).
> 
> I think wrapping them in a `unique_ptr` here is slightly simpler than implementing the right move semantics by hand, but LMK what you think.
> We could make them movable, though it's not absolutely trivial (we need an extra bool to indicate that this is moved-from and the destructor should be a no-op).
I kinda hate this part when dealing with movable objects, too. I still do that, mostly to avoid heap allocs, but we're not on a hot path, so it's fine both ways.
We could use `llvm::Optional` instead of `unique_ptr` to get rid of the heap alloc too.


================
Comment at: clangd/Trace.cpp:50
   // Contents must be a list of the other JSON key/values.
-  template <typename T> void event(StringRef Phase, const T &Contents) {
+  void event(StringRef Phase, json::obj &&Contents) {
     uint64_t TID = get_threadid();
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Any reason why we use rval-ref instead of accepting by-value?
> Two reasons I prefer this for json::expr/obj/arr:
> 
>  - major: you almost always want to pass a new literal, or move an existing one. Passing a `const Expr &` is usually a mistake, and taking `Expr&&`  makes it an error.
>  - minor: expr/obj/arr aren't trivially cheap to move. We forward these around internally, taking && at every level means we only pay for the move constructor once, pass-by-value pays on every call.
Makes sense, thanks for expanding on this. I'll make sure to pass around by r-value ref too when dealing with JSON objects.


https://reviews.llvm.org/D40132