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

On Thu, Jul 29, 2021 at 9:26 AM Jeff King [off-list ref] wrote:

On Wed, Jul 28, 2021 at 04:49:18PM -0600, Elijah Newren wrote:

quoted

On Mon, Jul 26, 2021 at 8:36 AM Derrick Stolee [off-list ref] wrote:

quoted

On 7/23/2021 8:54 AM, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

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.