[PATCH] D40489: [clangd] Changed tracing interfaces

[Pavel Sychev via Phabricator via cfe-commits]

luckygeck added inline comments.


================
Comment at: clangd/Trace.h:41
+  /// Context.
   static PtrKey<EventTracer> CtxKey;
 
----------------
ilya-biryukov wrote:
> 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?
I've just looked through the change adding Context and Key<>\PtrKey<> thingy. Yes, these classes are POD, so it is fine to have them static  (as you rely only on their address). 

Yes, I'd prefer to move the key out of `class` into a namespace - interface looks better without any data IMO.


================
Comment at: clangd/Trace.h:49
+  virtual void end_event(Context &Ctx, llvm::StringRef Name,
+                         json::obj &&Args) = 0;
+  /// Called for instant events.
----------------
ilya-biryukov wrote:
> 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?
> 
Oh, ok - setting tracer in a context only when we need to looks good enough.


https://reviews.llvm.org/D40489