luckygeck added inline comments.
Comment at: clangd/Trace.h:41
+ /// Context.
static PtrKey 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
