threads / rfc / 41710

RFC patchhashmap API: introduce for_each_hashmap_entry() helper macro

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

## tl;dr

4 messages between Mar 16, 2016 and Mar 16, 2016. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Alexander Kuleshov· Mar 16, 2016, 16:39 UTC · lore
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(-)
Show changes to 7 files +22 −22

Documentation/technical/api-hashmap.txt, builtin/describe.c, config.c, hashmap.c, hashmap.h, submodule-config.c, test-hashmap.c

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
Junio C Hamano· Mar 16, 2016, 18:09 UTC · re: Alexander Kuleshov · lore

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

Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 15 quoted lines
> 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.

Alexander Kuleshov· Mar 16, 2016, 18:39 UTC · re: Junio C Hamano · lore

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

Hello Junio,
On Thu, Mar 17, 2016 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> 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.
Karsten Blees· Mar 16, 2016, 23:47 UTC · re: Alexander Kuleshov · lore

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

Am 16.03.2016 um 17:39 schrieb Alexander Kuleshov:
Show 6 quoted lines
> 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);

← back to recent threads