On Tue, Oct 13, 2020 at 12:40:46AM +0000, Elijah Newren via GitGitGadget wrote:
> strmap_get_entry() is similar to strmap_get() except that instead of just
> returning the void* value that the string maps to, it returns the
> strmap_entry 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
Oh, I guess I should have read ahead when responding to the last patch. :)
Yes, this function makes perfect sense to have (along with the simpler
alternatives for the callers that don't need this complexity).
> strmap.c | 20 ++++++++++++++++++++
> strmap.h | 38 ++++++++++++++++++++++++++++++++++++++
The implementation all looks pretty straight-forward.
> +void strmap_remove(struct strmap *map, const char *str, int free_util)
> +{
> + struct strmap_entry entry, *ret;
> + hashmap_entry_init(&entry.ent, strhash(str));
> + entry.key = str;
> + ret = hashmap_remove_entry(&map->map, &entry, ent, NULL);
> + if (!ret)
> + return;
> + if (free_util)
> + free(ret->value);
> + if (map->strdup_strings)
> + free((char*)ret->key);
> + free(ret);
> +}
Another spot that would be simplified by using FLEXPTRs. :)
> +/*
> + * Return whether the strmap is empty.
> + */
> +static inline int strmap_empty(struct strmap *map)
> +{
> + return hashmap_get_size(&map->map) == 0;
> +}
Maybe:
return strmap_get_size(&map) == 0;
would be slightly simpler (and more importantly, show callers the
equivalence between the two).
> +/*
> + * iterate through @map using @iter, @var is a pointer to a type strmap_entry
> + */
> +#define strmap_for_each_entry(mystrmap, iter, var) \
> + for (var = hashmap_iter_first_entry_offset(&(mystrmap)->map, iter, \
> + OFFSETOF_VAR(var, ent)); \
> + var; \
> + var = hashmap_iter_next_entry_offset(iter, \
> + OFFSETOF_VAR(var, ent)))
Makes sense. This is like hashmap_for_each_entry, but we don't need
anyone to tell us the offset of "ent" within the struct.
I suspect we need the same "var = NULL" that hashmap recently got in
0ad621f61e (hashmap_for_each_entry(): workaround MSVC's runtime check
failure #3, 2020-09-30). Alternatively, I think you could drop
OFFSETOF_VAR completely in favor offsetof(struct strmap_entry, ent).
In fact, since we know the correct type for "var", we _could_ declare it
ourselves in a new block enclosing the loop. But that is probably making
the code too magic; people reading the code would say "huh? where is
entry declared?".
-Peff
