On Fri, Aug 21, 2020 at 06:52:24PM +0000, Elijah Newren via GitGitGadget wrote:
> Here I introduce a new strmap type, which my new merge backed, merge-ort,
> uses heavily. (I also made significant use of it in my changes to
> diffcore-rename). This strmap type was based on Peff's proposal from a
> couple years ago[1], but has additions that I made as I used it. I also
> start the series off with a quick documentation improvement to hashmap.c to
> differentiate between hashmap_free() and hashmap_free_entries(), since I
> personally had difficulty understanding them and it affects how
> strmap_clear()/strmap_free() are written.
I like the direction overall (unsurprisingly), but left a bunch of
comments. I do think if we're going to do this that it may be worth
cleaning up hashmap a bit first, especially around its clear/free
semantics, and its ability to lazy-allocate the table.
I'm happy to work on that, but don't want to step on your toes.
I also wonder if you looked at the khash stuff at all. Especially for
storing integers, it makes things much more natural. You'd do something
like:
/* you might even be able to just write !strcmp in the macro below */
static inline int streq(const char *a, const char *b)
{
return !strcmp(a, b);
}
KHASH_INIT(strint_map, char *, int, 1, strhash, streq);
and then you'd probably want a "put" wrapper that makes a copy of the
string. khash has its own charming awkwardness, but I'm just curious if you
looked at it and found it more awkward than hashmap.c, or if you just
didn't look at it.
-Peff
