Re: [PATCH] strbuf: add compound literal test balloon

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > 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,
> >         });
> >
> > The result is more readable as one can see the value of each member
> > of the object being passed to the function at the call site rather
> > than building the object piecemeal in the preceding lines.
> 
> Hmph, is it simpler, I have to wonder, in other words, I doubt you
> simplified the code.
> 
> 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.
> 
> 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.
> 
> 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.