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

From: Jeff King <peff@peff.net>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v3 00/13] Add struct strmap and associated utility functions
Date: Wed, 4 Nov 2020 15:52:50 -0500	[thread overview]
Message-ID: <20201104205250.GF3629238@coredump.intra.peff.net> (raw)
In-Reply-To: <pull.835.v3.git.git.1604343313.gitgitgadget@gmail.com>

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