[PATCH] D40489: [clangd] Changed tracing interfaces

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/Trace.h:41
+  /// Context.
   static PtrKey<EventTracer> CtxKey;
 
----------------
luckygeck wrote:
> This is a (1)static non-pod member in an (2) interface. Is it really a good idea? If we plan to have only one ctxkey, then maybe let's make it not bound to EventTracer?
It does not have any data members, has trivial default constructor and trivial destructor.
I don't think there are any problems we're gonna hit with this one, or am I missing something?

> then maybe let's make it not bound to EventTracer?
Do you propose to move it out of the `class` into the `clangd::trace` namespace instead?


================
Comment at: clangd/Trace.h:49
+  virtual void end_event(Context &Ctx, llvm::StringRef Name,
+                         json::obj &&Args) = 0;
+  /// Called for instant events.
----------------
luckygeck wrote:
> Maybe it is better to calculate these Args only if the tracing is enabled? This might be done at least this two ways:
> 1) Have `bool is_tracing_enabled()` method that we can check before calculating args.
> 2) Pass a function that will produce json::obj when called.
The current implementation won't compute args if `EventTracer` inside a `Context` is null, so I think this should cover our needs for per-request tracing (see `SPAN_ATTACH` macro).
But `is_tracing_enabled()` makes sense if we'd like to turn the tracing off in a middle of a single `Context` lifetime. This would make sense for global tracer, is this the use-case you anticipate?



https://reviews.llvm.org/D40489