On Wed, Mar 09, 2022 at 02:12:15PM -0500, Derrick Stolee wrote:
> On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:
>
> > +static int cmd_log_deinit(int ret, struct rev_info *rev)
> > +{
> > + release_revisions(rev);
> > + return ret;
> > +}
>
> This pattern of passing a return value through the helper
> function is a clever way to get around adding "int ret = ...;
> release(); return ret;" lines.
Clever indeed. I originally thought that cmd_log_deinit() was going to
be a wrapper around release_revisions() that also freed any log-specific
bits of the rev_info structure that are used exclusively by this file.
But that is not a great pattern, since having to track which file(s) use
which field(s) of the rev_info structure sounds cumbersome, error-prone,
and fragile. So I'm glad to see that it's really just used to
temporarily hold a return value.
I wouldn't be sad to see this as a macro, either, maybe something like:
#define LOG_TEARDOWN(ret, rev) \
do { release_revisions(rev); return ret; } while (0);
to make it clear(er) that this has a pretty narrow single purpose. But
even that doesn't seem like a great idea, since many of the uses have
the result of a function call as the first argument, which obviously
would not work with this macro.
So it's probably fine as-is ;).
Thanks,
Taylor
