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

[RFC PATCH] 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, some related variables 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                      | 9 ++++-----
 config.c                                | 6 ++----
 hashmap.c                               | 7 ++-----
 hashmap.h                               | 7 +++++++
 submodule-config.c                      | 6 +-----
 test-hashmap.c                          | 4 +---
 7 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt
index ad7a5bd..4c49aaf 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 hashmap with the certain
+	type of entry.
+
 `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..c678bbb 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -272,13 +272,12 @@ static void describe(const char *arg, int last_one)
 		fprintf(stderr, _("searching to describe %s\n"), arg);
 
 	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)) {
-			c = lookup_commit_reference_gently(n->peeled, 1);
+
+		for_each_hashmap_entry(&names, commit_name) {
+			c = lookup_commit_reference_gently(entry->peeled, 1);
 			if (c)
-				c->util = n;
+				c->util = entry;
 		}
 		have_util = 1;
 	}
diff --git a/config.c b/config.c
index 7ddb287..c4b09ad 100644
--- a/config.c
+++ b/config.c
@@ -1382,16 +1382,14 @@ 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;
 	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, config_set_element) {
 		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..0574326 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -140,11 +140,8 @@ void hashmap_free(struct hashmap *map, int free_entries)
 	if (!map || !map->table)
 		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);
+		for_each_hashmap_entry(map, hashmap_entry)
+			free(entry);
 	}
 	free(map->table);
 	memset(map, 0, sizeof(*map));
diff --git a/hashmap.h b/hashmap.h
index ab7958a..b8b158c 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)
 	return memintern(string, strlen(string));
 }
 
+#define for_each_hashmap_entry(map, type)		\
+	struct type *entry;				\
+	struct hashmap_iter iter;			\
+							\
+	hashmap_iter_init(map, &iter);			\
+	while ((entry = hashmap_iter_next(&iter)))
+
 #endif
diff --git a/submodule-config.c b/submodule-config.c
index b82d1fb..4be2812 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -65,16 +65,12 @@ static void free_one_config(struct submodule_entry *entry)
 
 static void cache_free(struct submodule_cache *cache)
 {
-	struct hashmap_iter iter;
-	struct submodule_entry *entry;
-
 	/*
 	 * We iterate over the name hash here to be symmetric with the
 	 * 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, submodule_entry)
 		free_one_config(entry);
 
 	hashmap_free(&cache->for_path, 1);
diff --git a/test-hashmap.c b/test-hashmap.c
index cc2891d..44758eb 100644
--- a/test-hashmap.c
+++ b/test-hashmap.c
@@ -224,9 +224,7 @@ 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, test_entry)
 				printf("%s %s\n", entry->key, get_value(entry));
 
 		} else if (!strcmp("size", cmd)) {
-- 
2.8.0.rc2.216.g1477fb2.dirty

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

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> diff --git a/hashmap.h b/hashmap.h
> index ab7958a..b8b158c 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)
>  	return memintern(string, strlen(string));
>  }
>  
> +#define for_each_hashmap_entry(map, type)		\
> +	struct type *entry;				\
> +	struct hashmap_iter iter;			\
> +							\
> +	hashmap_iter_init(map, &iter);			\
> +	while ((entry = hashmap_iter_next(&iter)))
> +

This is an easy way to introduce decl-after-statement, i.e. needs an
extra pair of {} around the thing.  It also forbids the callers from
defining "entry" and "iter" as their own identifier outside the
scope of this macro and use them inside the block that is iterated
over by shadowing these two variables.

Other than that, it looks like a good concept.  The syntax however
needs more thought because of the above two issues, I think.

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

[Alexander Kuleshov]

Hello Junio,

On Thu, Mar 17, 2016 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Kuleshov <kuleshovmail@gmail.com> writes:
>
>> diff --git a/hashmap.h b/hashmap.h
>> index ab7958a..b8b158c 100644
>> --- a/hashmap.h
>> +++ b/hashmap.h
>> @@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)
>>       return memintern(string, strlen(string));
>>  }
>>
>> +#define for_each_hashmap_entry(map, type)            \
>> +     struct type *entry;                             \
>> +     struct hashmap_iter iter;                       \
>> +                                                     \
>> +     hashmap_iter_init(map, &iter);                  \
>> +     while ((entry = hashmap_iter_next(&iter)))
>> +
>
> This is an easy way to introduce decl-after-statement, i.e. needs an
> extra pair of {} around the thing.  It also forbids the callers from
> defining "entry" and "iter" as their own identifier outside the
> scope of this macro and use them inside the block that is iterated
> over by shadowing these two variables.
>
> Other than that, it looks like a good concept.  The syntax however
> needs more thought because of the above two issues, I think.

Thanks for feedback. Will fix first issue and think about second.

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

[Karsten Blees]

Am 16.03.2016 um 17:39 schrieb 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
> 

The hashmap_iter_first() function allows you to do this instead:

	for (entry = hashmap_iter_first(map, &iter); entry; entry = hashmap_iter_next(&iter))
		doSomething(entry);

With an appropriate macro definition, this could be simplified to:

	#define hashmap_for_each(map, iter, entry) for (entry = hashmap_iter_first(map, iter); entry; entry = hashmap_iter_next(iter))
	...
	hashmap_for_each(map, &iter, entry)
		doSomething(entry);

You would still need to declare the 'iter' and 'entry' variables, but
there is no danger of decl-after-statement or variable shadowing
mentioned by Junio. That is, you can do this:

	hashmap_for_each(map, &iter, entry)
		if (checkCondition(entry))
			break;
	// work with found entry

Or even this:

	hashmap_for_each(map, &iter1, entry1)
		hashmap_for_each(map, &iter2, entry2)
			doSomething(entry1, entry2);