[PATCH] D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread.

[Sam McCall via Phabricator via cfe-commits]

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