On Fri, Oct 30, 2020 at 7:23 AM Jeff King <peff@xxxxxxxx> wrote:
>
> 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).
Makes sense; will change it.
> > +/*
> > + * 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?".
Actually, since we know ent is the first entry in strmap, the offset
is always 0. So can't we just avoid OFFSETOF_VAR() and offsetof()
entirely, by just using hashmap_iter_first() and hashmap_iter_next()?
I'm going to try that.
