On Tue, Nov 3, 2020 at 8:08 AM Jeff King <peff@xxxxxxxx> wrote:
>
> 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.
