On Mon, Nov 02, 2020 at 06:55:09PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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. :)
> +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.
> 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().
> +/*
> + * 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.
> +/*
> + * 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
