Re: [PATCH v5 12/15] strmap: enable allocations to come from a mem_pool

From: Jeff King <peff@peff.net>
Date: Nov 11, 2020, 19:01 UTC
Message-ID: <20201111190121.GE9902@coredump.intra.peff.net>
In-Reply-To: <fbaa60c4-ee6b-02b7-68b5-e5873f8ec713@gmail.com>

On Wed, Nov 11, 2020 at 05:33:47PM +0000, Phillip Wood wrote:

Show 18 quoted lines

> On 06/11/2020 00:24, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> > 
> > For heavy users of strmaps, allowing the keys and entries to be
> > allocated from a memory pool can provide significant overhead savings.
> > Add an option to strmap_init_with_options() to specify a memory pool.
> > [...]
> > diff --git a/strmap.h b/strmap.h
> > index c8c4d7c932..dda928703d 100644
> > --- a/strmap.h
> > +++ b/strmap.h
> > @@ -3,8 +3,10 @@
> >   #include "hashmap.h"
> > +struct mempool;
> 
> I think this is a typo - I assume you wanted to declare `struct mem_pool`
> but it's not strictly necessary as you're only adding a pointer to the
> struct below.

Good catch.

Even if we're only using a pointer to it, we still need a valid forward declaration (using the pointer only saves us from needing the full definition). Or so I thought.

It looks like the compiler will treat the use inside the struct:

Show 5 quoted lines

> >   struct strmap {
> >   	struct hashmap map;
> > +	struct mem_pool *pool;
> >   	unsigned int strdup_strings:1;
> >   };

as an implicit forward declaration, but not the ones inside the function declarations:

> >   void strmap_init_with_options(struct strmap *map,
> > +			      struct mem_pool *pool,
> >   			      int strdup_strings);

If you replace the pointer in the struct definition with "struct foo", then "make hdr-check" will complain about mem_pool in the function. And likewise if you replace the ones in the function with "struct foo", then we'll complain about those.

I'm not sure whether this is a seldom-seen corner of the C standard, or a compiler-specific thing (though both clang and gcc seem to allow it). At any rate, I think it is worth fixing the typo'd forward declaration (rather than deleting it) to make the intention clear.

-Peff