Re: [PATCH 05/22] sequencer: allow the sequencer to take custody of malloc()ed data
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 30, 2016, 07:29 UTC
- Message-ID
- <alpine.DEB.2.20.1608300915470.129229@virtualbox>
- In-Reply-To
- <733a899a-470b-79b3-b059-b38313a7057d@gmail.com>
Hi Kuba,
On Mon, 29 Aug 2016, Jakub Narębski wrote:
Show 15 quoted lines
> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze: > > > The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves > > like a one-shot command when it reads its configuration: memory is > > allocated and released only when the command exits. > > > > This is kind of okay for git-cherry-pick, which *is* a one-shot > > command. All the work to make the sequencer its work horse was done to > > allow using the functionality as a library function, though, including > > proper clean-up after use. > > > > This patch introduces an API to pass the responsibility of releasing > > certain memory to the sequencer. > > So how this API would be / is meant to be used?
I added an example to the commit message.
> Would sequencer as a library function be called multiple times, > or only once?
The point of a library function is that it should not care.
> I'm trying to find out how this is solved in other places of Git > code, and I have stumbled upon free_util in string_list...
I wanted this to be flexible enough to take care of any type of data, not just strings.
And while the string_list has a void *util field, it would be rather silly to add strings to a string list for the sole purpose of free()ing their util fields in the end.
(That was the conclusion I came to after a search of my own.)
Show 10 quoted lines
> > +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
> > +{
> > + ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> > + opts->owned[opts->owned_nr++] = set_me_free_after_use;
> > +
> > + return set_me_free_after_use;
>
> I was wondering what this 'set_me_free_after_use' parameter is about;
> wouldn't it be more readable if this parameter was called 'owned_data'
> or 'owned_ptr'?If I read "owned_ptr" as a function's parameter, I would assume that the associated memory is owned by the caller. So I would be puzzled reading that name.
Show 10 quoted lines
> > static void remove_sequencer_state(const struct replay_opts *opts)
> > {
> > struct strbuf dir = STRBUF_INIT;
> > + int i;
> > +
> > + for (i = 0; i < opts->owned_nr; i++)
> > + free(opts->owned[i]);
>
> I guess you can remove owned data in any order, regardless if you
> store struct or its members first...Indeed, this is not like a C++ destructor. It's free().
Show 15 quoted lines
> > diff --git a/sequencer.h b/sequencer.h
> > index c955594..20b708a 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -43,8 +43,14 @@ struct replay_opts {
> >
> > /* Only used by REPLAY_NONE */
> > struct rev_info *revs;
> > +
> > + /* malloc()ed data entrusted to the sequencer */
> > + void **owned;
> > + int owned_nr, owned_alloc;
>
> I'm not sure about naming conventions for those types of data, but
> wouldn't 'owned_data' be a better name? I could be wrong here...The convention seemed to be "void *X; int X_nr, X_alloc;", so I stuck with it.
Thanks for your review! Johannes