On 14/07/2025 15:26, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
A C99 compound literal creates an unnamed object whose value is given by
an initializer list. This allows us to simplify code where we cannot use
a designated initalizer because the values of some members of the object
need to be calculated first. For example this code from builtin/rebase.c
struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
struct reset_head_opts ropts = { 0 };
int ret;
strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
opts->reflog_action,
opts->head_name, oid_to_hex(&opts->onto->object.oid));
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
ropts.flags = RESET_HEAD_REFS_ONLY;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_head(the_repository, &ropts);
can be be simplified to
struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
int ret;
strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
opts->reflog_action,
opts->head_name, oid_to_hex(&opts->onto->object.oid));
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ret = reset_head(the_repository, &(struct reset_head_opts) {
.branch = opts->head_name,
.flags = RESET_HEAD_REFS_ONLY,
.branch_msg = branch_reflog.buf,
.head_msg = head_reflog.buf,
});
> One thing the above rewrite did is to make it clear to readers that
the struct instance is used just once and then immediately got
discarded. As long as the object that gets passed this way does not
hold resources that need to be discarded itself (in other words,
does not require a call to reset_head_opts_release()), it makes the
code easier to follow.
That's a good point - this example would not work if reset_head_opts()
took ownership of `branch_msg` and `head_msg`.
But once the struct gains members that need to be released, I am not
sure if this construct does not make it harder to spot leaks.
Somebody who adds a member to _release() to the struct presumably
audits and find all places that need to call _release(), but among
them they find this place---now what? They need to first convert it
to the old fashioned way and then call _release() after the
reset_head() call returns, I guess.
Another possibility is to do something like
struct reset_head_opts ropts;
/* ... */
ropts = (struct replay_head_opts) {
...
};
ret = reset_head(the_repository, &ropts);
reset_head_opts_release(&ropts);
which initializes all the members of `ropts` in one place though I'm not
sure if it is really better in practice.
I am not arguing against all uses of literals---I am just
anticipating future fallouts of encouraging overuse of this pattern,
and preparing what we would say when somebody adds a new use to
inappropriate places. Simple cases like the initialier should be
fine.
Yes, we'd want to be careful where we use them. I like the way we use
designated initializers and this gives us the opportunity to have a
similar style of initialization in a few more places.
Thanks
Phillip
