Re: [PATCH 6/9] trace2: convert ctx.thread_name to flex array

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2022, 18:03 UTC
Message-ID: <xmqq7d1eqhbf.fsf@gitster.g>
In-Reply-To: <6492b6d2b989e08bb539fff3ffe5bdf50fa0a195.1664900407.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Convert the `tr2tls_thread_ctx.thread_name` field from a `strbuf`
> to a "flex array" at the end of the context structure.
>
> The `thread_name` field is a constant string that is constructed when
> the context is created.  Using a (non-const) `strbuf` structure for it
> caused some confusion in the past because it implied that someone
> could rename a thread after it was created.  That usage was not
> intended.  Changing it to a "flex array" will hopefully make the
> intent more clear.

Surely, "const struct strbuf name;" member would be an oxymoron, and I agree that this should follow "use strbuf as an easy-to-work-with mechanism to come up with a string, and bake the final value into a struct as a member of type 'const char []'" pattern.

I recall saying why I thought the flex array was overkill, though.

You have been storing an up-to-24-byte human readable name by embedding a strbuf that has two size_t plus a pointer (i.e. 24-bytes even on Windows), and as TR2_MAX_THREAD_NAME is capped at 24 bytes anyway, an embedded fixed-size thread_name[TR2_MAX_THREAD_NAME+1] member may be the simplest thing to do, I suspect.

If we were to allow arbitrarily long thread_name[], which may not be a bad thing to do (e.g. we do not have to worry about truncation making two names ambiguous, for example), then the flex array is the right direction to go in, though.