On Wed, Nov 11, 2020 at 05:33:47PM +0000, Phillip Wood wrote:
> On 06/11/2020 00:24, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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:
> > 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
