Re: [PATCH v2 06/10] strmap: add more utility functions
- From
Jeff King <peff@peff.net>
- Date
- Oct 30, 2020, 14:23 UTC
- Message-ID
- <20201030142350.GF3277724@coredump.intra.peff.net>
- In-Reply-To
- <61b5bf11103a7bd12de8fd066e128c469da3a0a4.1602549650.git.gitgitgadget@gmail.com>
On Tue, Oct 13, 2020 at 12:40:46AM +0000, Elijah Newren via GitGitGadget wrote:
Show 9 quoted lines
> 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.
Show 14 quoted lines
> +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. :)
Show 7 quoted lines
> +/*
> + * 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).
Show 9 quoted lines
> +/* > + * 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