On Fri, Aug 21, 2020 at 06:52:27PM +0000, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@xxxxxxxxx> > > This adds a number of additional convienence functions I want/need: > * strmap_empty() > * strmap_get_size() > * strmap_remove() > * strmap_for_each_entry() > * strmap_free() > * strmap_get_item() > > I suspect the first four are self-explanatory. Yup, all make sense. We might also want real iterators rather than strmap_for_each_entry(), which can be a bit more convenient given the lack of lambdas in C. But I'd be happy to wait until a caller arises. > strmap_free() differs from strmap_clear() in that the data structure is > not reusable after it is called; strmap_clear() is not sufficient for > the API because without strmap_free() we will leak memory. Hmm, I missed in the previous function that strmap_clear() is actually leaving allocated memory. I think this is bad, because it's unlike most of our other data structure clear() functions. We could work around it with the lazy-init stuff I mentioned in my last email (i.e., _don't_ strmap_init() at the end of strmap_clear(), and just let strmap_put() take care of initializing if somebody actually adds something again). But IMHO this is a sign that we should be fixing hashmap() to work like that, too. > strmap_get_item() is similar to strmap_get() except that instead of just > returning the void* value that the string maps to, it returns the > string_list_item that contains both the string and the void* value (or > NULL if the string isn't in the map). This is helpful because it avoids > multiple lookups, e.g. in some cases a caller would need to call: > * strmap_contains() to check that the map has an entry for the string > * strmap_get() to get the void* value > * <do some work to update the value> > * strmap_put() to update/overwrite the value That makes sense. If you follow my suggestion to drop string_list_item, then it would be OK to return the whole str_entry. (I forgot to mention in the last patch, but perhaps strmap_entry would be a more distinctive name). > diff --git a/strmap.h b/strmap.h > index eb5807f6fa..45d0a4f714 100644 > --- a/strmap.h > +++ b/strmap.h > @@ -21,6 +21,11 @@ void strmap_init(struct strmap *map); > /* > * Remove all entries from the map, releasing any allocated resources. > */ > +void strmap_free(struct strmap *map, int free_values); > + > +/* > + * Same as calling strmap_free() followed by strmap_init(). > + */ > void strmap_clear(struct strmap *map, int free_values); I guess the docstring was a bit inaccurate in the previous patch, then. :) -Peff
