[PATCH 00/10] Friday night special: hash map cleanup

[PATCH 00/10] Friday night special: hash map cleanup

[Stefan Beller]

This goes on top of sb/hashmap-customize-comparison.
No functional impact but a pure cleanup series:
No casts to hashmap_cmp_fn in the whole code base any more.

This revealed another interesting thing, which is not a bug per se:
The const correctness of hashmap_cmp_fn as it requires all its void
pointers to be const!

We violate this in patch-ids.c as we modify the `fndata` at some
uncritical part (a part that doesn't change the equal-functionality
AFAICT).

Thanks,
Stefan

Stefan Beller (10):
  attr.c: drop hashmap_cmp_fn cast
  builtin/difftool.c: drop hashmap_cmp_fn cast
  builtin/describe: drop hashmap_cmp_fn cast
  config.c: drop hashmap_cmp_fn cast
  convert/sub-process: drop cast to hashmap_cmp_fn
  patch-ids.c: drop hashmap_cmp_fn cast
  remote.c: drop hashmap_cmp_fn cast
  submodule-config.c: drop hashmap_cmp_fn cast
  name-hash.c: drop hashmap_cmp_fn cast
  t/helper/test-hashmap: use custom data instead of duplicate cmp
    functions

 attr.c                  | 12 +++++++-----
 builtin/describe.c      |  9 ++++++---
 builtin/difftool.c      | 37 ++++++++++++++++++++++---------------
 config.c                | 10 ++++++----
 convert.c               |  3 +--
 name-hash.c             | 22 +++++++++++++---------
 patch-ids.c             | 14 +++++++++-----
 remote.c                | 12 ++++++++----
 sub-process.c           |  7 +++++--
 sub-process.h           |  4 ++--
 submodule-config.c      | 18 ++++++++++++------
 t/helper/test-hashmap.c | 34 ++++++++++++++++------------------
 12 files changed, 107 insertions(+), 75 deletions(-)

-- 
2.13.0.31.g9b732c453e

[PATCH 01/10] attr.c: drop hashmap_cmp_fn cast

[Stefan Beller]

MAke the code more readable and less error prone by avoiding the cast
of the compare function pointer in hashmap_init, but instead have the
correctly named void pointers to casted to the specific data structure.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 attr.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index 56961f0236..2f49151736 100644
--- a/attr.c
+++ b/attr.c
@@ -76,18 +76,20 @@ struct attr_hash_entry {
 };
 
 /* attr_hashmap comparison function */
-static int attr_hash_entry_cmp(void *unused_cmp_data,
-			       const struct attr_hash_entry *a,
-			       const struct attr_hash_entry *b,
-			       void *unused_keydata)
+static int attr_hash_entry_cmp(const void *unused_cmp_data,
+			       const void *entry,
+			       const void *entry_or_key,
+			       const void *unused_keydata)
 {
+	const struct attr_hash_entry *a = entry;
+	const struct attr_hash_entry *b = entry_or_key;
 	return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
 }
 
 /* Initialize an 'attr_hashmap' object */
 static void attr_hashmap_init(struct attr_hashmap *map)
 {
-	hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, NULL, 0);
+	hashmap_init(&map->map, attr_hash_entry_cmp, NULL, 0);
 }
 
 /*
-- 
2.13.0.31.g9b732c453e

[PATCH 02/10] builtin/difftool.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/difftool.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a1a26ba891..8864d846f8 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -131,10 +131,12 @@ struct working_tree_entry {
 };
 
 static int working_tree_entry_cmp(const void *unused_cmp_data,
-				  struct working_tree_entry *a,
-				  struct working_tree_entry *b,
-				  void *unused_keydata)
+				  const void *entry,
+				  const void *entry_or_key,
+				  const void *unused_keydata)
 {
+	const struct working_tree_entry *a = entry;
+	const struct working_tree_entry *b = entry_or_key;
 	return strcmp(a->path, b->path);
 }
 
@@ -149,9 +151,13 @@ struct pair_entry {
 };
 
 static int pair_cmp(const void *unused_cmp_data,
-		    struct pair_entry *a, struct pair_entry *b,
-		    void *unused_keydata)
+		    const void *entry,
+		    const void *entry_or_key,
+		    const void *unused_keydata)
 {
+	const struct pair_entry *a = entry;
+	const struct pair_entry *b = entry_or_key;
+
 	return strcmp(a->path, b->path);
 }
 
@@ -179,9 +185,13 @@ struct path_entry {
 };
 
 static int path_entry_cmp(const void *unused_cmp_data,
-			  struct path_entry *a, struct path_entry *b,
-			  void *key)
+			  const void *entry,
+			  const void *entry_or_key,
+			  const void *key)
 {
+	const struct path_entry *a = entry;
+	const struct path_entry *b = entry_or_key;
+
 	return strcmp(a->path, key ? key : b->path);
 }
 
@@ -372,10 +382,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	rdir_len = rdir.len;
 	wtdir_len = wtdir.len;
 
-	hashmap_init(&working_tree_dups,
-		     (hashmap_cmp_fn)working_tree_entry_cmp, NULL, 0);
-	hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, NULL, 0);
-	hashmap_init(&symlinks2, (hashmap_cmp_fn)pair_cmp, NULL, 0);
+	hashmap_init(&working_tree_dups, working_tree_entry_cmp, NULL, 0);
+	hashmap_init(&submodules, pair_cmp, NULL, 0);
+	hashmap_init(&symlinks2, pair_cmp, NULL, 0);
 
 	child.no_stdin = 1;
 	child.git_cmd = 1;
@@ -585,10 +594,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	 * in the common case of --symlinks and the difftool updating
 	 * files through the symlink.
 	 */
-	hashmap_init(&wt_modified, (hashmap_cmp_fn)path_entry_cmp,
-		     NULL, wtindex.cache_nr);
-	hashmap_init(&tmp_modified, (hashmap_cmp_fn)path_entry_cmp,
-		     NULL, wtindex.cache_nr);
+	hashmap_init(&wt_modified, path_entry_cmp, NULL, wtindex.cache_nr);
+	hashmap_init(&tmp_modified, path_entry_cmp, NULL, wtindex.cache_nr);
 
 	for (i = 0; i < wtindex.cache_nr; i++) {
 		struct hashmap_entry dummy;
-- 
2.13.0.31.g9b732c453e

[PATCH 03/10] builtin/describe: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/describe.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 8868f00ed0..f2f2edf935 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -55,10 +55,13 @@ static const char *prio_names[] = {
 };
 
 static int commit_name_cmp(const void *unused_cmp_data,
-			   const struct commit_name *cn1,
-			   const struct commit_name *cn2,
+			   const void *entry,
+			   const void *entry_or_key,
 			   const void *peeled)
 {
+	const struct commit_name *cn1 = entry;
+	const struct commit_name *cn2 = entry_or_key;
+
 	return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
 }
 
@@ -503,7 +506,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		return cmd_name_rev(args.argc, args.argv, prefix);
 	}
 
-	hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);
+	hashmap_init(&names, commit_name_cmp, NULL, 0);
 	for_each_rawref(get_name, NULL);
 	if (!names.size && !always)
 		die(_("No names found, cannot describe anything."));
-- 
2.13.0.31.g9b732c453e

[PATCH 04/10] config.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 config.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index 4a31e31ac3..30ff700629 100644
--- a/config.c
+++ b/config.c
@@ -1754,17 +1754,19 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 }
 
 static int config_set_element_cmp(const void *unused_cmp_data,
-				  const struct config_set_element *e1,
-				  const struct config_set_element *e2,
+				  const void *entry,
+				  const void *entry_or_key,
 				  const void *unused_keydata)
 {
+	const struct config_set_element *e1 = entry;
+	const struct config_set_element *e2 = entry_or_key;
+
 	return strcmp(e1->key, e2->key);
 }
 
 void git_configset_init(struct config_set *cs)
 {
-	hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp,
-		     NULL, 0);
+	hashmap_init(&cs->config_hash, config_set_element_cmp, NULL, 0);
 	cs->hash_initialized = 1;
 	cs->list.nr = 0;
 	cs->list.alloc = 0;
-- 
2.13.0.31.g9b732c453e

[PATCH 05/10] convert/sub-process: drop cast to hashmap_cmp_fn

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 convert.c     | 3 +--
 sub-process.c | 7 +++++--
 sub-process.h | 4 ++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/convert.c b/convert.c
index deaf0ba7b3..04966c723c 100644
--- a/convert.c
+++ b/convert.c
@@ -583,8 +583,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 
 	if (!subprocess_map_initialized) {
 		subprocess_map_initialized = 1;
-		hashmap_init(&subprocess_map, (hashmap_cmp_fn) cmd2process_cmp,
-			     NULL, 0);
+		hashmap_init(&subprocess_map, cmd2process_cmp, NULL, 0);
 		entry = NULL;
 	} else {
 		entry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);
diff --git a/sub-process.c b/sub-process.c
index a3cfab1a9d..6cbffa4406 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -6,10 +6,13 @@
 #include "pkt-line.h"
 
 int cmd2process_cmp(const void *unused_cmp_data,
-		    const struct subprocess_entry *e1,
-		    const struct subprocess_entry *e2,
+		    const void *entry,
+		    const void *entry_or_key,
 		    const void *unused_keydata)
 {
+	const struct subprocess_entry *e1 = entry;
+	const struct subprocess_entry *e2 = entry_or_key;
+
 	return strcmp(e1->cmd, e2->cmd);
 }
 
diff --git a/sub-process.h b/sub-process.h
index 96a2cca360..8cd07a59ab 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -21,8 +21,8 @@ struct subprocess_entry {
 /* subprocess functions */
 
 extern int cmd2process_cmp(const void *unused_cmp_data,
-			   const struct subprocess_entry *e1,
-			   const struct subprocess_entry *e2,
+			   const void *e1,
+			   const void *e2,
 			   const void *unused_keydata);
 
 typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
-- 
2.13.0.31.g9b732c453e

[PATCH 06/10] patch-ids.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 patch-ids.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index b4166b0f38..9ea523984b 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -35,11 +35,16 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
  * the side of safety.  The actual value being negative does not have
  * any significance; only that it is non-zero matters.
  */
-static int patch_id_cmp(struct diff_options *opt,
-			struct patch_id *a,
-			struct patch_id *b,
+static int patch_id_cmp(const void *cmpfn_data,
+			const void *entry,
+			const void *entry_or_key,
 			const void *unused_keydata)
 {
+	/* NEEDSWORK: const correctness? */
+	struct diff_options *opt = (void*)cmpfn_data;
+	struct patch_id *a = (void*)entry;
+	struct patch_id *b = (void*)entry_or_key;
+
 	if (is_null_oid(&a->patch_id) &&
 	    commit_patch_id(a->commit, opt, &a->patch_id, 0))
 		return error("Could not get patch ID for %s",
@@ -58,8 +63,7 @@ int init_patch_ids(struct patch_ids *ids)
 	ids->diffopts.detect_rename = 0;
 	DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
 	diff_setup_done(&ids->diffopts);
-	hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp,
-		     &ids->diffopts, 256);
+	hashmap_init(&ids->patches, patch_id_cmp, &ids->diffopts, 256);
 	return 0;
 }
 
-- 
2.13.0.31.g9b732c453e

[PATCH 07/10] remote.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 remote.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 60d0043921..3efa358558 100644
--- a/remote.c
+++ b/remote.c
@@ -134,10 +134,14 @@ struct remotes_hash_key {
 };
 
 static int remotes_hash_cmp(const void *unused_cmp_data,
-			    const struct remote *a,
-			    const struct remote *b,
-			    const struct remotes_hash_key *key)
+			    const void *entry,
+			    const void *entry_or_key,
+			    const void *keydata)
 {
+	const struct remote *a = entry;
+	const struct remote *b = entry_or_key;
+	const struct remotes_hash_key *key = keydata;
+
 	if (key)
 		return strncmp(a->name, key->str, key->len) || a->name[key->len];
 	else
@@ -147,7 +151,7 @@ static int remotes_hash_cmp(const void *unused_cmp_data,
 static inline void init_remotes_hash(void)
 {
 	if (!remotes_hash.cmpfn)
-		hashmap_init(&remotes_hash, (hashmap_cmp_fn)remotes_hash_cmp, NULL, 0);
+		hashmap_init(&remotes_hash, remotes_hash_cmp, NULL, 0);
 }
 
 static struct remote *make_remote(const char *name, int len)
-- 
2.13.0.31.g9b732c453e

[PATCH 08/10] submodule-config.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 0e1126183d..edc8dd04b6 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -35,27 +35,33 @@ static struct submodule_cache the_submodule_cache;
 static int is_cache_init;
 
 static int config_path_cmp(const void *unused_cmp_data,
-			   const struct submodule_entry *a,
-			   const struct submodule_entry *b,
+			   const void *entry,
+			   const void *entry_or_key,
 			   const void *unused_keydata)
 {
+	const struct submodule_entry *a = entry;
+	const struct submodule_entry *b = entry_or_key;
+
 	return strcmp(a->config->path, b->config->path) ||
 	       hashcmp(a->config->gitmodules_sha1, b->config->gitmodules_sha1);
 }
 
 static int config_name_cmp(const void *unused_cmp_data,
-			   const struct submodule_entry *a,
-			   const struct submodule_entry *b,
+			   const void *entry,
+			   const void *entry_or_key,
 			   const void *unused_keydata)
 {
+	const struct submodule_entry *a = entry;
+	const struct submodule_entry *b = entry_or_key;
+
 	return strcmp(a->config->name, b->config->name) ||
 	       hashcmp(a->config->gitmodules_sha1, b->config->gitmodules_sha1);
 }
 
 static void cache_init(struct submodule_cache *cache)
 {
-	hashmap_init(&cache->for_path, (hashmap_cmp_fn) config_path_cmp, NULL, 0);
-	hashmap_init(&cache->for_name, (hashmap_cmp_fn) config_name_cmp, NULL, 0);
+	hashmap_init(&cache->for_path, config_path_cmp, NULL, 0);
+	hashmap_init(&cache->for_name, config_name_cmp, NULL, 0);
 }
 
 static void free_one_config(struct submodule_entry *entry)
-- 
2.13.0.31.g9b732c453e

[PATCH 09/10] name-hash.c: drop hashmap_cmp_fn cast

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 name-hash.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index 0e10f3eab8..bd8dc7a6a7 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -17,10 +17,14 @@ struct dir_entry {
 };
 
 static int dir_entry_cmp(const void *unused_cmp_data,
-			 const struct dir_entry *e1,
-			 const struct dir_entry *e2,
-			 const char *name)
+			 const void *entry,
+			 const void *entry_or_key,
+			 const void *keydata)
 {
+	const struct dir_entry *e1 = entry;
+	const struct dir_entry *e2 = entry_or_key;
+	const char *name = keydata;
+
 	return e1->namelen != e2->namelen || strncasecmp(e1->name,
 			name ? name : e2->name, e1->namelen);
 }
@@ -110,10 +114,12 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
 }
 
 static int cache_entry_cmp(const void *unused_cmp_data,
-			   const struct cache_entry *ce1,
-			   const struct cache_entry *ce2,
+			   const void *entry,
+			   const void *entry_or_key,
 			   const void *remove)
 {
+	const struct cache_entry *ce1 = entry;
+	const struct cache_entry *ce2 = entry_or_key;
 	/*
 	 * For remove_name_hash, find the exact entry (pointer equality); for
 	 * index_file_exists, find all entries with matching hash code and
@@ -574,10 +580,8 @@ static void lazy_init_name_hash(struct index_state *istate)
 {
 	if (istate->name_hash_initialized)
 		return;
-	hashmap_init(&istate->name_hash, (hashmap_cmp_fn) cache_entry_cmp,
-			NULL, istate->cache_nr);
-	hashmap_init(&istate->dir_hash, (hashmap_cmp_fn) dir_entry_cmp,
-			NULL, istate->cache_nr);
+	hashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);
+	hashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);
 
 	if (lookup_lazy_params(istate)) {
 		hashmap_disallow_rehash(&istate->dir_hash, 1);
-- 
2.13.0.31.g9b732c453e

[PATCH 10/10] t/helper/test-hashmap: use custom data instead of duplicate cmp functions

[Stefan Beller]

With the new field that is passed to the compare function, we can pass
through flags there instead of having multiple compare functions.
Also drop the cast to hashmap_cmp_fn.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/helper/test-hashmap.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 095d7395f3..93ccfbb75f 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -13,20 +13,20 @@ static const char *get_value(const struct test_entry *e)
 	return e->key + strlen(e->key) + 1;
 }
 
-static int test_entry_cmp(const void *unused_cmp_data,
-			  const struct test_entry *e1,
-			  const struct test_entry *e2,
-			  const char* key)
+static int test_entry_cmp(const void *cmp_data,
+			  const void *entry,
+			  const void *entry_or_key,
+			  const void* keydata)
 {
-	return strcmp(e1->key, key ? key : e2->key);
-}
-
-static int test_entry_cmp_icase(const void *unused_cmp_data,
-				const struct test_entry *e1,
-				const struct test_entry *e2,
-				const char* key)
-{
-	return strcasecmp(e1->key, key ? key : e2->key);
+	const int ignore_case = cmp_data ? *((int*)cmp_data) : 0;
+	const struct test_entry *e1 = entry;
+	const struct test_entry *e2 = entry_or_key;
+	const char* key = keydata;
+
+	if (ignore_case)
+		return strcasecmp(e1->key, key ? key : e2->key);
+	else
+		return strcmp(e1->key, key ? key : e2->key);
 }
 
 static struct test_entry *alloc_test_entry(int hash, char *key, int klen,
@@ -96,8 +96,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
 	if (method & TEST_ADD) {
 		/* test adding to the map */
 		for (j = 0; j < rounds; j++) {
-			hashmap_init(&map, (hashmap_cmp_fn) test_entry_cmp,
-				     NULL, 0);
+			hashmap_init(&map, test_entry_cmp, NULL, 0);
 
 			/* add entries */
 			for (i = 0; i < TEST_SIZE; i++) {
@@ -109,7 +108,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)
 		}
 	} else {
 		/* test map lookups */
-		hashmap_init(&map, (hashmap_cmp_fn) test_entry_cmp, NULL, 0);
+		hashmap_init(&map, test_entry_cmp, NULL, 0);
 
 		/* fill the map (sparsely if specified) */
 		j = (method & TEST_SPARSE) ? TEST_SIZE / 10 : TEST_SIZE;
@@ -151,8 +150,7 @@ int cmd_main(int argc, const char **argv)
 
 	/* init hash map */
 	icase = argc > 1 && !strcmp("ignorecase", argv[1]);
-	hashmap_init(&map, (hashmap_cmp_fn) (icase ? test_entry_cmp_icase
-			: test_entry_cmp), NULL, 0);
+	hashmap_init(&map, test_entry_cmp, &icase, 0);
 
 	/* process commands from stdin */
 	while (fgets(line, sizeof(line), stdin)) {
-- 
2.13.0.31.g9b732c453e

Re: [PATCH 00/10] Friday night special: hash map cleanup

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This goes on top of sb/hashmap-customize-comparison.
> No functional impact but a pure cleanup series:
> No casts to hashmap_cmp_fn in the whole code base any more.
>
> This revealed another interesting thing, which is not a bug per se:
> The const correctness of hashmap_cmp_fn as it requires all its void
> pointers to be const!
>
> We violate this in patch-ids.c as we modify the `fndata` at some
> uncritical part (a part that doesn't change the equal-functionality
> AFAICT).

I am undecided, but perhaps we should loosen that, if some real-world
user has a legitimate need to take a mutable fndata?

I dunno.  The patches look good to me.

Thanks.