sammccall added inline comments.
================
Comment at: clangd/Trace.cpp:133
+ std::atomic<double> EndTime; // Filled in by endSpan().
+ std::string Name;
+ const uint64_t TID;
----------------
ilya-biryukov wrote:
> Maybe make all fields except `EndTime` const?
Good idea - what's safe to read vs write wasn't clear.
Ended up just making this a class and encapsulating EndTime instead though.
================
Comment at: clangd/Trace.h:46
+ // The Context returned by beginSpan is active, but Args is not ready.
+ virtual void endSpan() {};
----------------
ilya-biryukov wrote:
> It feels awkward that `endSpan()` does not have any parameters explicitly
> connecting it to `beginSpan()`.
> We could change `beginSpan()` to return a callback that would be called when
> `Span` ends instead, that would make the connection clear, albeit it's not
> very useful for `JSONTracer`.
>
> Not sure we should change it, just thinking out loud :-)
So destructor-of-context is still the preferred way of observing the end of an
event. endSpan() is kind of an attractive nuisance that we need for chrome
tracing.
Added comments to explain this.
Given that, I think we want a signature that:
- you basically can't use without first implementing and understanding the
primary interface
- can be defaulted to do nothing without interfering with the primary
interface (this also gives us backwards compatibility).
I'm not sure think it's a problem that this is a bit obscure...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43272
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
