Re: [PATCH v2 03/10] hashmap: allow re-use after hashmap_free()

From: Elijah Newren <newren@gmail.com>
Date: Nov 3, 2020, 16:16 UTC
Message-ID: <CABPp-BH7A7jXWm28+gpFHacMnteNN7gRLpWT6YaVqqkAffn8Mg@mail.gmail.com>
In-Reply-To: <20201103160818.GB444466@coredump.intra.peff.net>

On Tue, Nov 3, 2020 at 8:08 AM Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

>
> On Fri, Oct 30, 2020 at 08:37:42AM -0700, Elijah Newren wrote:
>
> > > This part I disagree with. If we did:
> > >
> > >   #define HASHMAP_INIT(fn, data) = { .cmpfn = cmpfn, cmpfn_data = data }
> > >
> > > then many callers could avoid handling the lazy-init themselves. E.g.:
> >
> > Ah, gotcha.  That makes sense to me.  Given that 43 out of 47 callers
> > of hashmap_init use cmpfn_data = NULL, should I shorten it to just one
> > parameter for the macro, and let the four special cases keep calling
> > hashmap_init() to specify a non-NULL cmpfn_data?
>
> I'd be fine with it either way. I actually wrote it without the data
> parameter at first, then changed my mine and added it in. ;)
>
> You could also do:
>
>   #define HASHMAP_INIT_DATA(fn, data) { .cmpfn = cmpfn, cmpfn_data = data }
>   #define HASHMAP_INIT(fn) HASHMAP_INIT_DATA(fn, NULL)
>
> if you want to keep most callers simple.

I ended up going with your HASHMAP_INIT(fn, data) in v3 that I submitted yesterday (except that you have a stray '=', are missing a '.' in front of cmpfn_data, and you'll trigger BUG()s if you don't also add .do_count_items = 1, but those are all minor fixups). In the future, if we determine we want/need the extra simplicity then we can always convert to this newer suggestion. I don't think it's that big a deal either way.