Re: [PATCH v2 06/10] strmap: add more utility functions

>
> 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?".