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