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
