Re: [PATCH v3 09/13] strmap: add functions facilitating use as a string->int map

[Jeff King <peff@peff.net>]

On Mon, Nov 02, 2020 at 06:55:09PM +0000, Elijah Newren via GitGitGadget wrote:

> 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. :)

> +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