Re: [PATCH 3/7] merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappers

From: Jeff King <peff@peff.net>
Date: Jul 30, 2021, 16:12 UTC
Message-ID: <YQQk7Vgf8hBQ9Ko+@coredump.intra.peff.net>
In-Reply-To: <CABPp-BF+1=yajJ_Z9MHHbeL6XkctfStj63xVXTkguNicx_PcPw@mail.gmail.com>

On Thu, Jul 29, 2021 at 08:27:51PM -0600, Elijah Newren wrote:

Show 19 quoted lines

> > 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?"

Yeah, "mem_pool_maybe_free" or something might explain it. But...

> 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.

That's fine, especially as dropping the conditiona USE_MEMORY_POOL flag means these functions will go away entirely.

-Peff