On Wed, Nov 4, 2020 at 12:52 PM Jeff King <peff@xxxxxxxx> wrote: > > 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. Cool, thanks for the careful reviews; I'll fix it up and send a v4 out. > > 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
