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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
