[PATCH] D40132: [clangd] Tracing improvements

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/JSONRPCDispatcher.h:78
   llvm::Optional<json::Expr> ID;
+  std::unique_ptr<trace::Span> Tracer;
 };
----------------
ilya-biryukov wrote:
> 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.
Oh, sorry, `llvm::Optional` surely doesn't help here, it would still be non-movable. Forget what I said.


https://reviews.llvm.org/D40132