[PATCH v2] hashmap API: introduce for_each_hashmap_entry() helper macro

[PATCH v2] hashmap API: introduce for_each_hashmap_entry() helper macro

[Alexander Kuleshov]

There is common pattern to traverse a hashmap in git source code:

        hashmap_iter_init(map, &iter);
        while ((entry = hashmap_iter_next(&iter)))
             // do something with entry

This patch introduces the for_each_hashmap_entry() macro for more
simple and clean usage of this pattern. It encapsulates loop over
a hashmap and makes bypass of a hashmap more readable.

This patch has not functioal changes, so behaviour still the same
as before.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 Documentation/technical/api-hashmap.txt | 5 +++++
 builtin/describe.c                      | 5 +++--
 config.c                                | 7 ++++---
 hashmap.c                               | 8 ++++----
 hashmap.h                               | 4 ++++
 submodule-config.c                      | 3 +--
 test-hashmap.c                          | 4 ++--
 7 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt
index ad7a5bd..7cb7d2a 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -193,6 +193,11 @@ more entries.
 `hashmap_iter_first` is a combination of both (i.e. initializes the iterator
 and returns the first entry, if any).
 
+`for_each_hashmap_entry`::
+
+	Allows iterate over entries of the given map with the given entry
+	and iterator.
+
 `const char *strintern(const char *string)`::
 `const void *memintern(const void *data, size_t len)`::
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 8a25abe..50e3377 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -274,8 +274,9 @@ static void describe(const char *arg, int last_one)
 	if (!have_util) {
 		struct hashmap_iter iter;
 		struct commit *c;
-		struct commit_name *n = hashmap_iter_first(&names, &iter);
-		for (; n; n = hashmap_iter_next(&iter)) {
+		struct commit_name *n;
+
+		for_each_hashmap_entry(&names, n, &iter) {
 			c = lookup_commit_reference_gently(n->peeled, 1);
 			if (c)
 				c->util = n;
diff --git a/config.c b/config.c
index 7ddb287..392d5a2 100644
--- a/config.c
+++ b/config.c
@@ -1382,16 +1382,17 @@ void git_configset_init(struct config_set *cs)
 
 void git_configset_clear(struct config_set *cs)
 {
-	struct config_set_element *entry;
 	struct hashmap_iter iter;
+	struct config_set_element *entry;
+
 	if (!cs->hash_initialized)
 		return;
 
-	hashmap_iter_init(&cs->config_hash, &iter);
-	while ((entry = hashmap_iter_next(&iter))) {
+	for_each_hashmap_entry(&cs->config_hash, entry, &iter) {
 		free(entry->key);
 		string_list_clear(&entry->value_list, 1);
 	}
+
 	hashmap_free(&cs->config_hash, 1);
 	cs->hash_initialized = 0;
 	free(cs->list.items);
diff --git a/hashmap.c b/hashmap.c
index b10b642..c41c12b 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -141,10 +141,10 @@ void hashmap_free(struct hashmap *map, int free_entries)
 		return;
 	if (free_entries) {
 		struct hashmap_iter iter;
-		struct hashmap_entry *e;
-		hashmap_iter_init(map, &iter);
-		while ((e = hashmap_iter_next(&iter)))
-			free(e);
+		struct hashmap_entry *entry;
+
+		for_each_hashmap_entry(map, entry, &iter)
+			free(entry);
 	}
 	free(map->table);
 	memset(map, 0, sizeof(*map));
diff --git a/hashmap.h b/hashmap.h
index ab7958a..772caf2 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -95,4 +95,8 @@ static inline const char *strintern(const char *string)
 	return memintern(string, strlen(string));
 }
 
+#define for_each_hashmap_entry(map, entry, iter)		\
+	for (entry = hashmap_iter_first(map, iter); entry;	\
+	     entry = hashmap_iter_next(iter))
+
 #endif
diff --git a/submodule-config.c b/submodule-config.c
index b82d1fb..5a8d7fa 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -73,8 +73,7 @@ static void cache_free(struct submodule_cache *cache)
 	 * allocation of struct submodule entries. Each is allocated by
 	 * their .gitmodule blob sha1 and submodule name.
 	 */
-	hashmap_iter_init(&cache->for_name, &iter);
-	while ((entry = hashmap_iter_next(&iter)))
+	for_each_hashmap_entry(&cache->for_name, entry, &iter)
 		free_one_config(entry);
 
 	hashmap_free(&cache->for_path, 1);
diff --git a/test-hashmap.c b/test-hashmap.c
index cc2891d..917d188 100644
--- a/test-hashmap.c
+++ b/test-hashmap.c
@@ -225,8 +225,8 @@ int main(int argc, char *argv[])
 		} else if (!strcmp("iterate", cmd)) {
 
 			struct hashmap_iter iter;
-			hashmap_iter_init(&map, &iter);
-			while ((entry = hashmap_iter_next(&iter)))
+
+			for_each_hashmap_entry(&map, entry, &iter)
 				printf("%s %s\n", entry->key, get_value(entry));
 
 		} else if (!strcmp("size", cmd)) {
-- 
2.8.0.rc3.212.g1f992f2.dirty

Re: [PATCH v2] hashmap API: introduce for_each_hashmap_entry() helper macro

[Karsten Blees]

Am 17.03.2016 um 11:38 schrieb Alexander Kuleshov:

> This patch introduces the for_each_hashmap_entry() macro for more

I'd rather call it 'hashmap_for_each', following the pattern
'operandtype_operation' used throughout git. E.g. we already have
'hashmap_get', not 'get_hashmap_entry'.

I realize that existing *for_each* implementations in the git code
base are a bit of a mess (except 'sha1_array_for_each_unique'). E.g.
there is 'for_each_string_list' and 'for_each_string_list_item'. Both
loop over the string_list_items of a string_list, but one is named
after the collection type, the other after the item type...IMO this
shouldn't set an example for future code.

The rest of the patch looks good to me.