{"thread":{"id":"41710","subject":"[RFC PATCH] hashmap API: introduce for_each_hashmap_entry() helper macro","startedAt":"2016-03-16T16:39:06Z","lastAt":"2016-03-16T23:47:52Z","messageCount":4,"participants":["Alexander Kuleshov","Junio C Hamano","Karsten Blees"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280916","messageId":"1458146346-27959-1-git-send-email-kuleshovmail@gmail.com","threadId":"41710","inReplyTo":null,"subject":"[RFC PATCH] hashmap API: introduce for_each_hashmap_entry() helper macro","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2016-03-16T16:39:06Z","receivedAt":"2016-03-16T16:39:06Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"There is common pattern to traverse a hashmap in git source code:\n\n        hashmap_iter_init(map, &iter);\n        while ((entry = hashmap_iter_next(&iter)))\n             // do something with entry\n\nThis patch introduces the for_each_hashmap_entry() macro for more\nsimple and clean usage of this pattern. It encapsulates loop over\na hashmap, some related variables and makes bypass of a hashmap\nmore readable.\n\nThis patch has not functioal changes, so behaviour still the same\nas before.\n\nSigned-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n---\n Documentation/technical/api-hashmap.txt | 5 +++++\n builtin/describe.c                      | 9 ++++-----\n config.c                                | 6 ++----\n hashmap.c                               | 7 ++-----\n hashmap.h                               | 7 +++++++\n submodule-config.c                      | 6 +-----\n test-hashmap.c                          | 4 +---\n 7 files changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt\nindex ad7a5bd..4c49aaf 100644\n--- a/Documentation/technical/api-hashmap.txt\n+++ b/Documentation/technical/api-hashmap.txt\n@@ -193,6 +193,11 @@ more entries.\n `hashmap_iter_first` is a combination of both (i.e. initializes the iterator\n and returns the first entry, if any).\n \n+`for_each_hashmap_entry`::\n+\n+\tAllows iterate over entries of the given hashmap with the certain\n+\ttype of entry.\n+\n `const char *strintern(const char *string)`::\n `const void *memintern(const void *data, size_t len)`::\n \ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 8a25abe..c678bbb 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -272,13 +272,12 @@ static void describe(const char *arg, int last_one)\n \t\tfprintf(stderr, _(\"searching to describe %s\\n\"), arg);\n \n \tif (!have_util) {\n-\t\tstruct hashmap_iter iter;\n \t\tstruct commit *c;\n-\t\tstruct commit_name *n = hashmap_iter_first(&names, &iter);\n-\t\tfor (; n; n = hashmap_iter_next(&iter)) {\n-\t\t\tc = lookup_commit_reference_gently(n->peeled, 1);\n+\n+\t\tfor_each_hashmap_entry(&names, commit_name) {\n+\t\t\tc = lookup_commit_reference_gently(entry->peeled, 1);\n \t\t\tif (c)\n-\t\t\t\tc->util = n;\n+\t\t\t\tc->util = entry;\n \t\t}\n \t\thave_util = 1;\n \t}\ndiff --git a/config.c b/config.c\nindex 7ddb287..c4b09ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -1382,16 +1382,14 @@ void git_configset_init(struct config_set *cs)\n \n void git_configset_clear(struct config_set *cs)\n {\n-\tstruct config_set_element *entry;\n-\tstruct hashmap_iter iter;\n \tif (!cs->hash_initialized)\n \t\treturn;\n \n-\thashmap_iter_init(&cs->config_hash, &iter);\n-\twhile ((entry = hashmap_iter_next(&iter))) {\n+\tfor_each_hashmap_entry(&cs->config_hash, config_set_element) {\n \t\tfree(entry->key);\n \t\tstring_list_clear(&entry->value_list, 1);\n \t}\n+\n \thashmap_free(&cs->config_hash, 1);\n \tcs->hash_initialized = 0;\n \tfree(cs->list.items);\ndiff --git a/hashmap.c b/hashmap.c\nindex b10b642..0574326 100644\n--- a/hashmap.c\n+++ b/hashmap.c\n@@ -140,11 +140,8 @@ void hashmap_free(struct hashmap *map, int free_entries)\n \tif (!map || !map->table)\n \t\treturn;\n \tif (free_entries) {\n-\t\tstruct hashmap_iter iter;\n-\t\tstruct hashmap_entry *e;\n-\t\thashmap_iter_init(map, &iter);\n-\t\twhile ((e = hashmap_iter_next(&iter)))\n-\t\t\tfree(e);\n+\t\tfor_each_hashmap_entry(map, hashmap_entry)\n+\t\t\tfree(entry);\n \t}\n \tfree(map->table);\n \tmemset(map, 0, sizeof(*map));\ndiff --git a/hashmap.h b/hashmap.h\nindex ab7958a..b8b158c 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)\n \treturn memintern(string, strlen(string));\n }\n \n+#define for_each_hashmap_entry(map, type)\t\t\\\n+\tstruct type *entry;\t\t\t\t\\\n+\tstruct hashmap_iter iter;\t\t\t\\\n+\t\t\t\t\t\t\t\\\n+\thashmap_iter_init(map, &iter);\t\t\t\\\n+\twhile ((entry = hashmap_iter_next(&iter)))\n+\n #endif\ndiff --git a/submodule-config.c b/submodule-config.c\nindex b82d1fb..4be2812 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -65,16 +65,12 @@ static void free_one_config(struct submodule_entry *entry)\n \n static void cache_free(struct submodule_cache *cache)\n {\n-\tstruct hashmap_iter iter;\n-\tstruct submodule_entry *entry;\n-\n \t/*\n \t * We iterate over the name hash here to be symmetric with the\n \t * allocation of struct submodule entries. Each is allocated by\n \t * their .gitmodule blob sha1 and submodule name.\n \t */\n-\thashmap_iter_init(&cache->for_name, &iter);\n-\twhile ((entry = hashmap_iter_next(&iter)))\n+\tfor_each_hashmap_entry(&cache->for_name, submodule_entry)\n \t\tfree_one_config(entry);\n \n \thashmap_free(&cache->for_path, 1);\ndiff --git a/test-hashmap.c b/test-hashmap.c\nindex cc2891d..44758eb 100644\n--- a/test-hashmap.c\n+++ b/test-hashmap.c\n@@ -224,9 +224,7 @@ int main(int argc, char *argv[])\n \n \t\t} else if (!strcmp(\"iterate\", cmd)) {\n \n-\t\t\tstruct hashmap_iter iter;\n-\t\t\thashmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = hashmap_iter_next(&iter)))\n+\t\t\tfor_each_hashmap_entry(&map, test_entry)\n \t\t\t\tprintf(\"%s %s\\n\", entry->key, get_value(entry));\n \n \t\t} else if (!strcmp(\"size\", cmd)) {\n-- \n2.8.0.rc2.216.g1477fb2.dirty\n"},{"id":"280937","messageId":"xmqq37rq5m79.fsf@gitster.mtv.corp.google.com","threadId":"41710","inReplyTo":"1458146346-27959-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [RFC PATCH] hashmap API: introduce for_each_hashmap_entry() helper macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-16T18:09:30Z","receivedAt":"2016-03-16T18:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n\n> diff --git a/hashmap.h b/hashmap.h\n> index ab7958a..b8b158c 100644\n> --- a/hashmap.h\n> +++ b/hashmap.h\n> @@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)\n>  \treturn memintern(string, strlen(string));\n>  }\n>  \n> +#define for_each_hashmap_entry(map, type)\t\t\\\n> +\tstruct type *entry;\t\t\t\t\\\n> +\tstruct hashmap_iter iter;\t\t\t\\\n> +\t\t\t\t\t\t\t\\\n> +\thashmap_iter_init(map, &iter);\t\t\t\\\n> +\twhile ((entry = hashmap_iter_next(&iter)))\n> +\n\nThis is an easy way to introduce decl-after-statement, i.e. needs an\nextra pair of {} around the thing.  It also forbids the callers from\ndefining \"entry\" and \"iter\" as their own identifier outside the\nscope of this macro and use them inside the block that is iterated\nover by shadowing these two variables.\n\nOther than that, it looks like a good concept.  The syntax however\nneeds more thought because of the above two issues, I think.\n"},{"id":"280942","messageId":"CANCZXo4uePVvk8_h2KuZQe4UFcFS1C76JvOfugK2nH3oH7TOsw@mail.gmail.com","threadId":"41710","inReplyTo":"xmqq37rq5m79.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] hashmap API: introduce for_each_hashmap_entry() helper macro","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2016-03-16T18:39:55Z","receivedAt":"2016-03-16T18:39:55Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Junio,\n\nOn Thu, Mar 17, 2016 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Alexander Kuleshov <kuleshovmail@gmail.com> writes:\n>\n>> diff --git a/hashmap.h b/hashmap.h\n>> index ab7958a..b8b158c 100644\n>> --- a/hashmap.h\n>> +++ b/hashmap.h\n>> @@ -95,4 +95,11 @@ static inline const char *strintern(const char *string)\n>>       return memintern(string, strlen(string));\n>>  }\n>>\n>> +#define for_each_hashmap_entry(map, type)            \\\n>> +     struct type *entry;                             \\\n>> +     struct hashmap_iter iter;                       \\\n>> +                                                     \\\n>> +     hashmap_iter_init(map, &iter);                  \\\n>> +     while ((entry = hashmap_iter_next(&iter)))\n>> +\n>\n> This is an easy way to introduce decl-after-statement, i.e. needs an\n> extra pair of {} around the thing.  It also forbids the callers from\n> defining \"entry\" and \"iter\" as their own identifier outside the\n> scope of this macro and use them inside the block that is iterated\n> over by shadowing these two variables.\n>\n> Other than that, it looks like a good concept.  The syntax however\n> needs more thought because of the above two issues, I think.\n\nThanks for feedback. Will fix first issue and think about second.\n"},{"id":"280989","messageId":"56E9F0A8.5080308@gmail.com","threadId":"41710","inReplyTo":"1458146346-27959-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [RFC PATCH] hashmap API: introduce for_each_hashmap_entry() helper macro","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2016-03-16T23:47:52Z","receivedAt":"2016-03-16T23:47:52Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 16.03.2016 um 17:39 schrieb Alexander Kuleshov:\n\n> There is common pattern to traverse a hashmap in git source code:\n> \n>         hashmap_iter_init(map, &iter);\n>         while ((entry = hashmap_iter_next(&iter)))\n>              // do something with entry\n> \n\nThe hashmap_iter_first() function allows you to do this instead:\n\n\tfor (entry = hashmap_iter_first(map, &iter); entry; entry = hashmap_iter_next(&iter))\n\t\tdoSomething(entry);\n\nWith an appropriate macro definition, this could be simplified to:\n\n\t#define hashmap_for_each(map, iter, entry) for (entry = hashmap_iter_first(map, iter); entry; entry = hashmap_iter_next(iter))\n\t...\n\thashmap_for_each(map, &iter, entry)\n\t\tdoSomething(entry);\n\nYou would still need to declare the 'iter' and 'entry' variables, but\nthere is no danger of decl-after-statement or variable shadowing\nmentioned by Junio. That is, you can do this:\n\n\thashmap_for_each(map, &iter, entry)\n\t\tif (checkCondition(entry))\n\t\t\tbreak;\n\t// work with found entry\n\nOr even this:\n\n\thashmap_for_each(map, &iter1, entry1)\n\t\thashmap_for_each(map, &iter2, entry2)\n\t\t\tdoSomething(entry1, entry2);\n"}]}