On 12/28/21 8:48 PM, Ævar Arnfjörð Bjarmason wrote:
On Tue, Dec 28 2021, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
Teach the message formatter in the Trace2 perf target to accept an
optional thread name argument. This will override the thread name
inherited from the thread local storage data block.
This will be used in a future commit for global events that should
not be tied to a particular thread, such as a global stopwatch timer.
We already have a "ctx", and that "ctx" has a "thread_name", but here
and in the preceding commit we're adding a "thread_name" to every caller
of these functions in case we'd like to override it.
Wouldn't it make more sense to just pass a "ctx" to these functions? One
of them already takes it, here's an (obviously incomplete) fixup on top
of your series to make the one that doesn't take a "ctx", and for the
only non-NULL users of "thread_name" to just use a trivial helper to
pass in a "ctx" with a new "thread_name", then to swap it back.
It would make for a smaller diffstat for this already large series, or
we could do exactly what we're doing now, but avoid the churn of
adjusting every caller by introducing a new sister function for those
who want this parameter to be non-NULL.
I suppose it is possible to have a helper version of
`event_fmt_prepare()` that takes the extra argument and
fixup the existing function to call it with NULL.
I'll see if that makes sense.
[...]
+static void event_fmt_prepare_tn(const char *event_name, const char *file,
+ int line, const struct repository *repo,
+ struct json_writer *jw,
+ const char *thread_name)
+{
+ struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
+ const char *tmp;
+
+ tmp = ctx->thread_name;
+ ctx->thread_name = thread_name;
+ event_fmt_prepare(event_name, __FILE__, __LINE__, NULL, jw, ctx);
+ ctx->thread_name = tmp;
+}
[...]
This only works if we agree that thread name is a pointer inside
the structure and not a flex-array.
Personally, I think this is trying to do things backwards by
temporarily changing the ctx->thread_name field. I think it
would be better to `event_fmt_prepare_tn()` do the actual
work with the supplied thread name and have the existing
`event_fmt_prepare()` just call it with ctx->thread_name.
Then we don't need to hack up the ctx.
I'll see if this makes the diffs a little cleaner.
Jeff
