Re: [PATCH v3 09/13] strmap: add functions facilitating use as a string->int map
- From
Jeff King <peff@peff.net>
- Date
- Nov 4, 2020, 20:21 UTC
- Message-ID
- <20201104202152.GB3629238@coredump.intra.peff.net>
- In-Reply-To
- <c1d217217180cff0b048d5dbac57b8e363db7848.1604343314.git.gitgitgadget@gmail.com>
On Mon, Nov 02, 2020 at 06:55:09PM +0000, Elijah Newren via GitGitGadget wrote:
Show 15 quoted lines
> From: Elijah Newren <newren@gmail.com> > > Although strmap could be used as a string->int map, one either had to > allocate an int for every entry and then deallocate later, or one had to > do a bunch of casting between (void*) and (intptr_t). > > Add some special functions that do the casting. Also, rename put->set > for such wrapper functions since 'put' implied there may be some > deallocation needed if the string was already found in the map, which > isn't the case when we're storing an int value directly in the void* > slot instead of using the void* slot as a pointer to data. > > A note on the name: if anyone has a better name suggestion than > strintmap, I'm happy to take it. It seems slightly unwieldy, but I have > not been able to come up with a better name.
You can probably drop this last paragraph. It's good for review, but probably not in the commit message. :)
Show 10 quoted lines
> +void strintmap_incr(struct strintmap *map, const char *str, intptr_t amt)
> +{
> + struct strmap_entry *entry = find_strmap_entry(&map->map, str);
> + if (entry) {
> + intptr_t *whence = (intptr_t*)&entry->value;
> + *whence += amt;
> + }
> + else
> + strintmap_set(map, str, map->default_value + amt);
> +}Here we use the new default_value. Neat.
Show 13 quoted lines
> diff --git a/strmap.h b/strmap.h
> index 10b4642860..31474f781e 100644
> --- a/strmap.h
> +++ b/strmap.h
> @@ -23,6 +23,11 @@ int cmp_strmap_entry(const void *hashmap_cmp_fn_data,
> .map = HASHMAP_INIT(cmp_strmap_entry, NULL), \
> .strdup_strings = 1, \
> }
> +#define STRINTMAP_INIT { \
> + .map.map = HASHMAP_INIT(cmp_strmap_entry, NULL), \
> + .map.strdup_strings = 1, \
> + .default_value = 0, \
> + }Re-using STRMAP_INIT would shorten this (and avoid repeating internal details of how strmap works). Like:
#define STRINTMAP_INIT { \
.map = STRMAP_INIT, \
.default_value = 0, \
}You can also omit default_value, as the value of any un-mentioned elements will get the usual C zero-initialization. So:
#define STRINTMAP_INIT { .map = STRMAP_INIT }would be sufficient (though I don't mind making the .default_value part explicit). It could also be a parameter to the macro, but I suspect it would be rarely used. I don't mind leaving it as something that advanced callers can get from using strintmap_init().
Show 13 quoted lines
> +/* > + * strintmap: > + * A map of string -> int, typecasting the void* of strmap to an int. > + * > + * Primary differences: > + * 1) Since the void* value is just an int in disguise, there is no value > + * to free. (Thus one fewer argument to strintmap_clear) > + * 2) strintmap_get() returns an int; it also requires an extra parameter to > + * be specified so it knows what value to return if the underlying strmap > + * has not key matching the given string. > + * 3) No strmap_put() equivalent; strintmap_set() and strintmap_incr() > + * instead. > + */
I think (2) here is out-of-date, as we now use map->default_value.
Show 11 quoted lines
> +/*
> + * Returns the value for str in the map. If str isn't found in the map,
> + * the map's default_value is returned.
> + */
> +static inline int strintmap_get(struct strintmap *map, const char *str)
> +{
> + struct strmap_entry *result = strmap_get_entry(&map->map, str);
> + if (!result)
> + return map->default_value;
> + return (intptr_t)result->value;
> +}And we get to reuse default_value here again. Nice.
-Peff