Re: [PATCH 3/7] merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappers
- From
Elijah Newren <newren@gmail.com>
- Date
- Jul 30, 2021, 02:27 UTC
- Message-ID
- <CABPp-BF+1=yajJ_Z9MHHbeL6XkctfStj63xVXTkguNicx_PcPw@mail.gmail.com>
- In-Reply-To
- <YQLInKdjor6Vs1yE@coredump.intra.peff.net>
On Thu, Jul 29, 2021 at 9:26 AM Jeff King <peff@peff.net> wrote:
Show 37 quoted lines
>
> On Wed, Jul 28, 2021 at 04:49:18PM -0600, Elijah Newren wrote:
>
> > On Mon, Jul 26, 2021 at 8:36 AM Derrick Stolee <stolee@gmail.com> wrote:
> > >
> > > On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:
> > > > From: Elijah Newren <newren@gmail.com>
> > > >
> > > > We need functions which will either call
> > > > xmalloc, xcalloc, xstrndup
> > > > or
> > > > mem_pool_alloc, mem_pool_calloc, mem_pool_strndup
> > > > depending on whether we have a non-NULL memory pool. Add these
> > > > functions; the next commit will make use of these.
> > >
> > > I briefly considered that this should just be the way the
> > > mem_pool_* methods work. It does rely on the caller knowing
> > > to free() the allocated memory when their pool is NULL, so
> > > perhaps such a universal change might be too much. What do
> > > you think?
> >
> > That's interesting, but I'm worried it might be a bit much. Do others
> > on the list have an opinion here?
>
> FWIW, I had the same thought. You can also provide a helper to make the
> freeing side nicer:
>
> static void mem_pool_free(struct mem_pool *m, void *ptr)
> {
> if (m)
> return; /* will be freed when pool frees */
> free(ptr);
> }
>
> We do something similar with unuse_commit_buffer(), where the caller
> isn't aware of we pulled the buffer from cache or allocated it
> especially for them.Having a paired function may help one side, but I worry that the name (mem_pool_free) might introduce some confusion of its own -- "Why is there a mem_pool_free() function, isn't the point of memory pools to not need to individually free things?" Or, "Why are they freeing the pool here and what's the extra parameter?"
I'm not sure I see the right way to address that, so I think I'm going to leave this part out of my series and let someone else add such changes on top if they feel motivated to do so.