Subject: Re: [PATCH v2 19/19] hashmap: remove type arg from hashmap_{get,put,remove}_entry

From: Derrick Stolee <[email-masked]>
Date: Wed, 25 Sep 2019 08:42:54 -0400

On 9/23/2019 9:03 PM, Eric Wong wrote:
> Since these macros already take a `keyvar' pointer of a known type,
> we can rely on OFFSETOF_VAR to get the correct offset without
> relying on non-portable `__typeof__' and `offsetof'.
> 
> Argument order is also rearranged, so `keyvar' and `member' are
> sequential as they are used as: `keyvar->member'
> 
> Signed-off-by: Eric Wong <[email-masked]>
> ---
>  attr.c                  |  2 +-
>  blame.c                 | 10 ++++-----
>  builtin/difftool.c      |  2 +-
>  builtin/fast-export.c   |  2 +-
>  config.c                |  3 +--
>  diff.c                  |  6 ++----
>  hashmap.c               |  2 +-
>  hashmap.h               | 45 ++++++++++++++++++++++++++++++-----------
>  merge-recursive.c       |  6 ++----
>  name-hash.c             |  3 +--
>  patch-ids.c             |  3 +--
>  range-diff.c            |  4 +---
>  remote.c                |  3 +--
>  revision.c              |  3 +--
>  sub-process.c           |  3 +--
>  submodule-config.c      | 10 +++------
>  t/helper/test-hashmap.c |  4 +---
>  17 files changed, 56 insertions(+), 55 deletions(-)
> 
> diff --git a/attr.c b/attr.c
> index 9849106627..15f0efdf60 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -103,7 +103,7 @@ static void *attr_hashmap_get(struct attr_hashmap *map,
>  	hashmap_entry_init(&k.ent, memhash(key, keylen));
>  	k.key = key;
>  	k.keylen = keylen;
> -	e = hashmap_get_entry(&map->map, &k, NULL, struct attr_hash_entry, ent);
> +	e = hashmap_get_entry(&map->map, &k, ent, NULL);
>  
>  	return e ? e->value : NULL;
>  }
> diff --git a/blame.c b/blame.c
> index 90b247abf9..6384f48133 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -419,8 +419,8 @@ static void get_fingerprint(struct fingerprint *result,
>  			continue;
>  		hashmap_entry_init(&entry->entry, hash);
>  
> -		found_entry = hashmap_get_entry(&result->map, entry, NULL,
> -					struct fingerprint_entry, entry);
> +		found_entry = hashmap_get_entry(&result->map, entry,
> +						/* member name */ entry, NULL);

In case I forget to point this out during the rest of my review: this
use of "/* member name */" to distinguish between the two "entry"
strings is very helpful for review. Likely, it will help future code
authors.

I looked at PATCHes 18 & 19 to see the end-result before going through
the rest. These are nice mechanical changes that present a cleaner API.
A worthy goal.

Thanks,
-Stolee