[PATCH] D40132: [clangd] Tracing improvements

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/JSONRPCDispatcher.h:78
   llvm::Optional<json::Expr> ID;
+  std::unique_ptr<trace::Span> Tracer;
 };
----------------
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.


================
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();
----------------
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.


https://reviews.llvm.org/D40132