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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
