Re: [PATCH v3 00/13] Add struct strmap and associated utility functions

>
> On Mon, Nov 02, 2020 at 06:55:00PM +0000, Elijah Newren via GitGitGadget wrote:
>
> > Changes since v2 (almost all of which were suggestions from Peff):
>
> Thanks again for your work on this series, and your willingness to
> listen to my various suggestions. ;)
>
> This mostly looks good to me. I pointed out a few minor nits in reply to
> individual patches, but there's at least one correctness problem, so
> we'll need a v4.

> > Things that I'm still unsure about:
> >
> >  * strintmap_init() takes a default_value parameter, as suggested by Peff.
> >    But this makes the function name strintmap_init_with_options() weird,
> >    because strintmap_init() already takes one option, so it seems like the
> >    name needs to replace "options" with "more_options". But that's kinda
> >    ugly too. I'm guessing strintmap_init_with_options() is fine as-is, but
> >    I'm wondering if anyone else thinks it looks weird and if so if there is
> >    anything I should do about it.
>
> You could drop default_value from strintmap_init_with_options(). I'd
> _guess_ most callers would be happy with 0, but you'd know much better
> than I what your first crop of callers will want.
>
> I'm happy with it either way.
>
> > Things Peff mentioned on v2 that I did NOT do:
> >
> >  * Peff brought up some questions about mapping strintmap to an int rather
> >    than an unsigned or intptr_t. I discussed my rationale in the thread
>
> Yeah, I'm well convinced that what you have here is fine.
>
> > Things Peff mentioned on v1 that are still not included and which Peff
> > didn't comment on for v2, but which may still be worth mentioning again:
> >
> >  * Peff brought up the idea of having a free_values member instead of having
> >    a free_values parameter to strmap_clear(). That'd just mean moving the
> >    parameter from strmap_clear() to strmap_init() and would be easy to do,
> >    but he sounded like he was just throwing it out as an idea and I didn't
> >    have a strong opinion, so I left it as-is. If others have
> >    opinions/preferences, changing it is easy right now.
>
> Yeah, I was mostly thinking out loud. What you have here looks fine to
> me.
>
> >  * Peff early on wanted the strmap_entry type to have a char key[FLEX_ALLOC]
> >    instead of having a (const) char *key. I spent a couple more days on this
> >    despite him not mentioning it while reviewing v2, and finally got it
> >    working this time and running valgrind-free. Note that such a change
> >    means always copying the key instead of allowing it as an option. After
> >    implementing it, I timed it and it slowed down my important testcase by
> >    just over 6%. So I chucked it. I think the FLEXPTR_ALLOC_STR usage in
> >    combination with defaulting to strdup_strings=1 gives us most the
> >    benefits Peff wanted, while still allowing merge-ort to reuse strings
> >    when it's important.
>
> Yes, I'd agree that FLEXPTR is a good middle ground. If I really manage
> to find a caller later where I think the complexity might be worth
> saving a few bytes, perhaps I'll try it then and get some real
> measurements. My guess is that won't ever actually happen. :)
>
> -Peff