Re: [PATCH 3/5] strmap: add more utility functions

From: Jeff King <peff@peff.net>
Date: Aug 21, 2020, 19:58 UTC
Message-ID: <20200821195822.GE1165@coredump.intra.peff.net>
In-Reply-To: <5bda171d0c1562573800d4bcbfac4799f01d9c20.1598035949.git.gitgitgadget@gmail.com>

On Fri, Aug 21, 2020 at 06:52:27PM +0000, Elijah Newren via GitGitGadget wrote:

Show 11 quoted lines

> From: Elijah Newren <newren@gmail.com>
> 
> This adds a number of additional convienence functions I want/need:
>   * strmap_empty()
>   * strmap_get_size()
>   * strmap_remove()
>   * strmap_for_each_entry()
>   * strmap_free()
>   * strmap_get_item()
> 
> I suspect the first four are self-explanatory.

Yup, all make sense. We might also want real iterators rather than strmap_for_each_entry(), which can be a bit more convenient given the lack of lambdas in C. But I'd be happy to wait until a caller arises.

> strmap_free() differs from strmap_clear() in that the data structure is
> not reusable after it is called; strmap_clear() is not sufficient for
> the API because without strmap_free() we will leak memory.

Hmm, I missed in the previous function that strmap_clear() is actually leaving allocated memory. I think this is bad, because it's unlike most of our other data structure clear() functions.

We could work around it with the lazy-init stuff I mentioned in my last email (i.e., _don't_ strmap_init() at the end of strmap_clear(), and just let strmap_put() take care of initializing if somebody actually adds something again).

But IMHO this is a sign that we should be fixing hashmap() to work like that, too.

Show 9 quoted lines

> strmap_get_item() is similar to strmap_get() except that instead of just
> returning the void* value that the string maps to, it returns the
> string_list_item 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

That makes sense. If you follow my suggestion to drop string_list_item, then it would be OK to return the whole str_entry. (I forgot to mention in the last patch, but perhaps strmap_entry would be a more distinctive name).

Show 14 quoted lines

> diff --git a/strmap.h b/strmap.h
> index eb5807f6fa..45d0a4f714 100644
> --- a/strmap.h
> +++ b/strmap.h
> @@ -21,6 +21,11 @@ void strmap_init(struct strmap *map);
>  /*
>   * Remove all entries from the map, releasing any allocated resources.
>   */
> +void strmap_free(struct strmap *map, int free_values);
> +
> +/*
> + * Same as calling strmap_free() followed by strmap_init().
> + */
>  void strmap_clear(struct strmap *map, int free_values);

I guess the docstring was a bit inaccurate in the previous patch, then. :)

-Peff