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