{"thread":{"id":"53186","subject":"[PATCH 1/2] oidmap: make oidmap_free independent of struct layout","startedAt":"2020-04-08T04:08:15Z","lastAt":"2020-04-25T03:00:57Z","messageCount":10,"participants":["Abhishek Kumar","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"395057","messageId":"20200408040659.14511-1-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":null,"subject":"[PATCH 1/2] oidmap: make oidmap_free independent of struct layout","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-08T04:06:58Z","receivedAt":"2020-04-08T04:08:15Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"c8e424c introduced hashmap_free_entries, which can free any struct\npointer, regardless of the hashmap_entry field offset.\n\noidmap does not make use of this flexibilty, hardcoding the offset to\nzero instead. Let's fix this by passing struct type and member to\nhashmap_free_entries.\n\nAdditionally, removes an erroneous semi-colon at the end of\nhashmap_free_entries macro.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n hashmap.h | 2 +-\n oidmap.c  | 6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/hashmap.h b/hashmap.h\nindex 79ae9f80de..6d0a65a39f 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);\n  * where @member is the hashmap_entry struct used to associate with @map\n  */\n #define hashmap_free_entries(map, type, member) \\\n-\thashmap_free_(map, offsetof(type, member));\n+\thashmap_free_(map, offsetof(type, member))\n \n /* hashmap_entry functions */\n \ndiff --git a/oidmap.c b/oidmap.c\nindex 423aa014a3..65d63787a8 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)\n \tif (!map)\n \t\treturn;\n \n-\t/* TODO: make oidmap itself not depend on struct layouts */\n-\thashmap_free_(&map->map, free_entries ? 0 : -1);\n+\tif (free_entries)\n+\t\thashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);\n+\telse\n+\t\thashmap_free(&map->map);\n }\n \n void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n-- \n2.26.0\n\n"},{"id":"395058","messageId":"20200408040659.14511-2-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200408040659.14511-1-abhishekkumar8222@gmail.com","subject":"[PATCH 2/2] oidmap: rework iterators to return typed pointer","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-08T04:06:59Z","receivedAt":"2020-04-08T04:08:19Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"87571c3 modified hashmap_iter_next to return a hashmap_entry pointer\ninstead of void pointer.\n\nHowever, oidmap_iter_next is unaware of the struct type containing\noidmap_entry and explicilty returns a void pointer.\n\nRework oidmap_iter_* to include struct type and return appropriate\npointer. This allows for compile-time type checks.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n oidmap.h               | 27 +++++++++++++++------------\n t/helper/test-oidmap.c |  2 +-\n 2 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/oidmap.h b/oidmap.h\nindex c66a83ab1d..5d6b34a7ce 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter\n \thashmap_iter_init(&map->map, &iter->h_iter);\n }\n \n-static inline void *oidmap_iter_next(struct oidmap_iter *iter)\n-{\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)hashmap_iter_next(&iter->h_iter);\n-}\n+/*\n+ * Returns the next entry, or NULL if there are no more entries.\n+ *\n+ * The entry is of @type (e.g. \"struct foo\") and has a member of type struct\n+ * oidmap_entry.\n+ */\n+#define oidmap_iter_next(iter, type) \\\n+\t(type *) hashmap_iter_next(&(iter)->h_iter)\n \n-static inline void *oidmap_iter_first(struct oidmap *map,\n-\t\t\t\t      struct oidmap_iter *iter)\n-{\n-\toidmap_iter_init(map, iter);\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)oidmap_iter_next(iter);\n-}\n+/*\n+ * Returns the first entry in @map using @iter, where the entry is of @type\n+ * (e.g. \"struct foo\") and has a member of type struct oidmap_entry.\n+ */\n+#define oidmap_iter_first(map, iter, type) \\\n+\t({oidmap_iter_init(map, iter); \\\n+\t oidmap_iter_next(iter, type); })\n \n #endif\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 0acf99931e..a28bf007a8 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)\n \n \t\t\tstruct oidmap_iter iter;\n \t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n+\t\t\twhile ((entry = oidmap_iter_next(&iter, struct test_entry)))\n \t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n \n \t\t} else {\n-- \n2.26.0\n\n"},{"id":"395061","messageId":"xmqqr1wyplaf.fsf@gitster.c.googlers.com","threadId":"53186","inReplyTo":"20200408040659.14511-1-abhishekkumar8222@gmail.com","subject":"Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-08T06:02:32Z","receivedAt":"2020-04-08T06:02:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhishek Kumar <abhishekkumar8222@gmail.com> writes:\n\n> c8e424c introduced hashmap_free_entries, which can free any struct\n> pointer, regardless of the hashmap_entry field offset.\n\n    c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)\n    introduced hashmap_free_entries(), which ...\n\nis how we refer to existing commits and functions.\n\n> oidmap does not make use of this flexibilty, hardcoding the offset to\n> zero instead. Let's fix this by passing struct type and member to\n> hashmap_free_entries.\n\nMakes sense.\n\n> Additionally, removes an erroneous semi-colon at the end of\n> hashmap_free_entries macro.\n\ns/removes/remove/\n\nGood eyes.  Of course, your if/else would have broken without this\nfix ;-)\n\nLooking good.  Thanks.\n\n> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n> ---\n>  hashmap.h | 2 +-\n>  oidmap.c  | 6 ++++--\n>  2 files changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/hashmap.h b/hashmap.h\n> index 79ae9f80de..6d0a65a39f 100644\n> --- a/hashmap.h\n> +++ b/hashmap.h\n> @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);\n>   * where @member is the hashmap_entry struct used to associate with @map\n>   */\n>  #define hashmap_free_entries(map, type, member) \\\n> -\thashmap_free_(map, offsetof(type, member));\n> +\thashmap_free_(map, offsetof(type, member))\n>  \n>  /* hashmap_entry functions */\n>  \n> diff --git a/oidmap.c b/oidmap.c\n> index 423aa014a3..65d63787a8 100644\n> --- a/oidmap.c\n> +++ b/oidmap.c\n> @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)\n>  \tif (!map)\n>  \t\treturn;\n>  \n> -\t/* TODO: make oidmap itself not depend on struct layouts */\n> -\thashmap_free_(&map->map, free_entries ? 0 : -1);\n> +\tif (free_entries)\n> +\t\thashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);\n> +\telse\n> +\t\thashmap_free(&map->map);\n>  }\n>  \n>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n"},{"id":"395062","messageId":"20200408070346.24872-1-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200408040659.14511-1-abhishekkumar8222@gmail.com","subject":"[PATCH v2 1/2] oidmap: make oidmap_free independent of struct layout","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-08T07:03:45Z","receivedAt":"2020-04-08T07:05:06Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06)\nintroduced hashmap_free_entries(), which can free any struct pointer,\nregardless of the hashmap_entry field offset.\n\noidmap does not make use of this flexibilty, hardcoding the offset to\nzero instead. Let's fix this by passing struct type and member to\nhashmap_free_entries().\n\nAdditionally, remove an erroneous semi-colon at the end of\nhashmap_free_entries() macro.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n\nChanges in v2:\n- Fix references to earlier commits.\n- Grammar fixes.\n\n hashmap.h | 2 +-\n oidmap.c  | 6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/hashmap.h b/hashmap.h\nindex 79ae9f80de..6d0a65a39f 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);\n  * where @member is the hashmap_entry struct used to associate with @map\n  */\n #define hashmap_free_entries(map, type, member) \\\n-\thashmap_free_(map, offsetof(type, member));\n+\thashmap_free_(map, offsetof(type, member))\n \n /* hashmap_entry functions */\n \ndiff --git a/oidmap.c b/oidmap.c\nindex 423aa014a3..65d63787a8 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries)\n \tif (!map)\n \t\treturn;\n \n-\t/* TODO: make oidmap itself not depend on struct layouts */\n-\thashmap_free_(&map->map, free_entries ? 0 : -1);\n+\tif (free_entries)\n+\t\thashmap_free_entries(&map->map, struct oidmap_entry, internal_entry);\n+\telse\n+\t\thashmap_free(&map->map);\n }\n \n void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n-- \n2.26.0\n\n"},{"id":"395063","messageId":"20200408070346.24872-2-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200408070346.24872-1-abhishekkumar8222@gmail.com","subject":"[PATCH v2 2/2] oidmap: rework iterators to return typed pointer","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-08T07:03:46Z","receivedAt":"2020-04-08T07:05:08Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified\nhashmap_iter_next() to return a hashmap_entry pointer instead of void\npointer.\n\nHowever, oidmap_iter_next() is unaware of the struct type containing\noidmap_entry and explicitly returns a void pointer.\n\nRework oidmap_iter_next() to include struct type and return appropriate\npointer. This allows for compile-time type checks.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n oidmap.h               | 27 +++++++++++++++------------\n t/helper/test-oidmap.c |  2 +-\n 2 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/oidmap.h b/oidmap.h\nindex c66a83ab1d..5d6b34a7ce 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter\n \thashmap_iter_init(&map->map, &iter->h_iter);\n }\n \n-static inline void *oidmap_iter_next(struct oidmap_iter *iter)\n-{\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)hashmap_iter_next(&iter->h_iter);\n-}\n+/*\n+ * Returns the next entry, or NULL if there are no more entries.\n+ *\n+ * The entry is of @type (e.g. \"struct foo\") and has a member of type struct\n+ * oidmap_entry.\n+ */\n+#define oidmap_iter_next(iter, type) \\\n+\t(type *) hashmap_iter_next(&(iter)->h_iter)\n \n-static inline void *oidmap_iter_first(struct oidmap *map,\n-\t\t\t\t      struct oidmap_iter *iter)\n-{\n-\toidmap_iter_init(map, iter);\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)oidmap_iter_next(iter);\n-}\n+/*\n+ * Returns the first entry in @map using @iter, where the entry is of @type\n+ * (e.g. \"struct foo\") and has a member of type struct oidmap_entry.\n+ */\n+#define oidmap_iter_first(map, iter, type) \\\n+\t({oidmap_iter_init(map, iter); \\\n+\t oidmap_iter_next(iter, type); })\n \n #endif\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 0acf99931e..a28bf007a8 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)\n \n \t\t\tstruct oidmap_iter iter;\n \t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n+\t\t\twhile ((entry = oidmap_iter_next(&iter, struct test_entry)))\n \t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n \n \t\t} else {\n-- \n2.26.0\n\n"},{"id":"395101","messageId":"20200408215423.GA3468797@coredump.intra.peff.net","threadId":"53186","inReplyTo":"20200408040659.14511-1-abhishekkumar8222@gmail.com","subject":"Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-08T21:54:23Z","receivedAt":"2020-04-08T21:54:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 08, 2020 at 09:36:58AM +0530, Abhishek Kumar wrote:\n\n> c8e424c introduced hashmap_free_entries, which can free any struct\n> pointer, regardless of the hashmap_entry field offset.\n> \n> oidmap does not make use of this flexibilty, hardcoding the offset to\n> zero instead. Let's fix this by passing struct type and member to\n> hashmap_free_entries.\n\nI'm not sure how much this improves anything.\n\nWe're now telling the hashmap code how to get to our \"struct\noidmap_entry\" pointer by using the correct offset there. But we know\nthat offset will always be 0, because we put our internal hashmap entry\nat the start of oidmap_entry.\n\nWhat you can't do is:\n\n  struct foo {\n    int first_member;\n    struct oidmap_entry not_the_first_member;\n  };\n\nand work directly with \"struct foo\" pointers. You have to convert\noidmap_entry pointers into foo pointers with container_of() or similar.\nAnd that is true both before and after your patch.\n\nThat said, I don't mind doing this as a cleanup; there's a subtle\ndependency on the location of internal_entry within object_entry,\nand this would move towards getting rid of it.\n\n-Peff\n"},{"id":"395103","messageId":"20200408220840.GB3468797@coredump.intra.peff.net","threadId":"53186","inReplyTo":"20200408070346.24872-2-abhishekkumar8222@gmail.com","subject":"Re: [PATCH v2 2/2] oidmap: rework iterators to return typed pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-08T22:08:40Z","receivedAt":"2020-04-08T22:08:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 08, 2020 at 12:33:46PM +0530, Abhishek Kumar wrote:\n\n> 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified\n> hashmap_iter_next() to return a hashmap_entry pointer instead of void\n> pointer.\n> \n> However, oidmap_iter_next() is unaware of the struct type containing\n> oidmap_entry and explicitly returns a void pointer.\n> \n> Rework oidmap_iter_next() to include struct type and return appropriate\n> pointer. This allows for compile-time type checks.\n\nYes, I think returning a pointer to an oidmap_entry makes sense. And\nthen we get type safety, and anybody who wants embed an oidmap_entry can\nuse container_of() to get back to their original struct.\n\nBut...\n\n> +/*\n> + * Returns the next entry, or NULL if there are no more entries.\n> + *\n> + * The entry is of @type (e.g. \"struct foo\") and has a member of type struct\n> + * oidmap_entry.\n> + */\n> +#define oidmap_iter_next(iter, type) \\\n> +\t(type *) hashmap_iter_next(&(iter)->h_iter)\n\nThis cast is turning a hashmap_entry into whatever type the caller\npassed in.  But it's doing it with a straight cast. We know that\nhashmap_entry and oidmap_entry pointers are equivalent, but we don't\nknow where the oidmap_entry is with respect to the user's type.\n\nI think oidmap_iter_next() should continue to be a function that returns\nan oidmap_entry pointer (and use container_of_or_null() to get to it\nfrom the hashmap_entry, even though we know the offset is 0).\n\nAnd then the caller can either use container_of() to get to their\noriginal struct, or we can provide a helper macro. See the difference\nbetween hashmap_iter_first() and hashmap_iter_first_entry().\n\nThere's no hashmap_iter_next_entry(). There could be, but instead it\nskipped straight to hashmap_for_each_entry(), which uses an offset\nwithin the variable rather than the type. But likewise, we could add\noidmap_for_each_entry() here.\n\n> diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\n> index 0acf99931e..a28bf007a8 100644\n> --- a/t/helper/test-oidmap.c\n> +++ b/t/helper/test-oidmap.c\n> @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv)\n>  \n>  \t\t\tstruct oidmap_iter iter;\n>  \t\t\toidmap_iter_init(&map, &iter);\n> -\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n> +\t\t\twhile ((entry = oidmap_iter_next(&iter, struct test_entry)))\n>  \t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n\nThis works because \"test_entry\" has the oidmap_entry at the start.\n\nBut it wouldn't work with a struct where that wasn't the case, nor would\nit provide any compile-time safety (because of the cast).\n\nNote that if we do want to support that and get type safety (and I think\nit is worth doing), oidmap_get() would need similar treatment (it\nreturns a void pointer, but it is really a pointer to an oidmap_entry).\nAnd I guess oidmap_put() and oidmap_remove(), which returns pointers to\nexisting entries.\n\n-Peff\n"},{"id":"396226","messageId":"20200425025921.1397-1-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200408040659.14511-1-abhishekkumar8222@gmail.com","subject":"[PATCH v3 1/3] oidmap: make oidmap_free independent of struct layout","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-25T02:59:19Z","receivedAt":"2020-04-25T03:00:47Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"c8e424c introduced hashmap_free_entries, which can free any struct\npointer, regardless of the hashmap_entry field offset.\n\noidmap does not make use of this flexibilty, hardcoding the offset to\nzero instead. Let's fix this by passing struct type and member to\nhashmap_free_entries.\n\nAdditionally, remove an erroneous semi-colon at the end of\nhashmap_free_entries macro.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n hashmap.h | 2 +-\n oidmap.c  | 7 +++++--\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/hashmap.h b/hashmap.h\nindex 79ae9f80de..6d0a65a39f 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset);\n  * where @member is the hashmap_entry struct used to associate with @map\n  */\n #define hashmap_free_entries(map, type, member) \\\n-\thashmap_free_(map, offsetof(type, member));\n+\thashmap_free_(map, offsetof(type, member))\n \n /* hashmap_entry functions */\n \ndiff --git a/oidmap.c b/oidmap.c\nindex 423aa014a3..44345a8cf2 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -26,8 +26,11 @@ void oidmap_free(struct oidmap *map, int free_entries)\n \tif (!map)\n \t\treturn;\n \n-\t/* TODO: make oidmap itself not depend on struct layouts */\n-\thashmap_free_(&map->map, free_entries ? 0 : -1);\n+\tif (free_entries)\n+\t\thashmap_free_entries(\n+\t\t\t&map->map, struct oidmap_entry, internal_entry);\n+\telse\n+\t\thashmap_free(&map->map);\n }\n \n void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n-- \n2.26.0\n\n"},{"id":"396227","messageId":"20200425025921.1397-2-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200425025921.1397-1-abhishekkumar8222@gmail.com","subject":"[PATCH v3 2/3] oidmap: rework iterators to return typed pointer","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-25T02:59:20Z","receivedAt":"2020-04-25T03:00:54Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified\nhashmap_iter_next() to return a hashmap_entry pointer instead of void\npointer.\n\nHowever, oidmap_iter_next() is unaware of the struct type containing\noidmap_entry and explicitly returns a void pointer.\n\nRewrite oidmap_iter_next() to return oidmap_entry pointer and add\nhelper macros to return pointers to container struct.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n oidmap.h               | 41 +++++++++++++++++++++++++++++++++++------\n t/helper/test-oidmap.c |  7 ++++---\n 2 files changed, 39 insertions(+), 9 deletions(-)\n\ndiff --git a/oidmap.h b/oidmap.h\nindex c66a83ab1d..d79b087ab0 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -66,6 +66,8 @@ void *oidmap_put(struct oidmap *map, void *entry);\n  */\n void *oidmap_remove(struct oidmap *map, const struct object_id *key);\n \n+#define oidmap_entry_from_hashmap_entry(entry) \\\n+\tcontainer_of_or_null(entry, struct oidmap_entry, internal_entry)\n \n struct oidmap_iter {\n \tstruct hashmap_iter h_iter;\n@@ -76,18 +78,45 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter\n \thashmap_iter_init(&map->map, &iter->h_iter);\n }\n \n-static inline void *oidmap_iter_next(struct oidmap_iter *iter)\n+/* Returns the next oidmap_entry, or NULL if there are no more entries. */\n+static inline struct oidmap_entry *oidmap_iter_next(struct oidmap_iter *iter)\n {\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)hashmap_iter_next(&iter->h_iter);\n+\treturn oidmap_entry_from_hashmap_entry(\n+\t\t\thashmap_iter_next(&iter->h_iter));\n }\n \n-static inline void *oidmap_iter_first(struct oidmap *map,\n+/* Initializes the iterator and returns the first entry, if any. */\n+static inline struct oidmap_entry *oidmap_iter_first(struct oidmap *map,\n \t\t\t\t      struct oidmap_iter *iter)\n {\n \toidmap_iter_init(map, iter);\n-\t/* TODO: this API could be reworked to do compile-time type checks */\n-\treturn (void *)oidmap_iter_next(iter);\n+\treturn oidmap_iter_next(iter);\n }\n \n+/*\n+ * Returns the first entry in @map using @iter, where the entry if of\n+ * @type (e.g. \"struct foo\") and @member is the name of the\n+ * \"struct oidmap_entry\" in @type\n+ */\n+#define oidmap_iter_first_entry(map, iter, type, member) \\\n+\tcontainer_of_or_null(oidmap_iter_first(map, iter), type, member)\n+\n+/* Internal macro for oidmap_for_each_entry */\n+#define oidmap_iter_next_entry_offset(iter, offset) \\\n+\tcontainer_of_or_null_offset(oidmap_iter_next(iter), offset)\n+\n+/* Internal macro for oidmap_for_each_entry */\n+#define oidmap_iter_first_entry_offset(map, iter, offset) \\\n+\tcontainer_of_or_null_offset(oidmap_iter_first(map, iter), offset)\n+\n+/*\n+ * Iterate through @map using @iter, @var is a pointer to a type\n+ * containing a @member which is a \"struct oidmap_entry\"\n+ */\n+#define oidmap_for_each_entry(map, iter, var, member) \\\n+\tfor (var = oidmap_iter_first_entry_offset(map, iter, \\\n+\t\t\t\t\t\tOFFSETOF_VAR(var, member)); \\\n+\t\tvar; \\\n+\t\tvar = oidmap_iter_next_entry_offset(iter, \\\n+\t\t\t\t\t\tOFFSETOF_VAR(var, member)))\n #endif\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 0acf99931e..3f599b21b8 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -95,9 +95,10 @@ int cmd__oidmap(int argc, const char **argv)\n \t\t} else if (!strcmp(\"iterate\", cmd)) {\n \n \t\t\tstruct oidmap_iter iter;\n-\t\t\toidmap_iter_init(&map, &iter);\n-\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n-\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n+\t\t\tstruct test_entry *ent;\n+\n+\t\t\toidmap_for_each_entry(&map, &iter, ent, entry)\n+\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&ent->entry.oid), ent->name);\n \n \t\t} else {\n \n-- \n2.26.0\n\n"},{"id":"396228","messageId":"20200425025921.1397-3-abhishekkumar8222@gmail.com","threadId":"53186","inReplyTo":"20200425025921.1397-1-abhishekkumar8222@gmail.com","subject":"[PATCH v3 3/3] oidmap: return oidmap_entry pointers","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-04-25T02:59:21Z","receivedAt":"2020-04-25T03:00:57Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"oidmap_entry is usually embedded as first member of user data\nstructures. Since oidmap is unaware of structure, it returns an void\npointer for oidmap_get(), oidmap_put() and oidmap_remove().\n\nThis pointer is rather a pointer to a oidmap entry. Teach the functions\nto return a oidmap_entry pointer and add helper macros to return\npointers to container struct.\n\nSigned-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>\n---\n list-objects-filter.c  |  8 +++++---\n oidmap.c               | 21 ++++++++++++---------\n oidmap.h               | 42 ++++++++++++++++++++++++++++++++++++------\n replace-object.c       |  7 ++++---\n sequencer.c            | 28 ++++++++++++++++++----------\n t/helper/test-oidmap.c |  8 +++++---\n 6 files changed, 80 insertions(+), 34 deletions(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 1e8d4e763d..16b45d913e 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -183,13 +183,15 @@ static enum list_objects_filter_result filter_trees_depth(\n \t\treturn include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;\n \n \tcase LOFS_BEGIN_TREE:\n-\t\tseen_info = oidmap_get(\n-\t\t\t&filter_data->seen_at_depth, &obj->oid);\n+\t\toidmap_get_entry(seen_info, base, &filter_data->seen_at_depth,\n+\t\t\t\t &obj->oid);\n+\n \t\tif (!seen_info) {\n \t\t\tseen_info = xcalloc(1, sizeof(*seen_info));\n \t\t\toidcpy(&seen_info->base.oid, &obj->oid);\n \t\t\tseen_info->depth = filter_data->current_depth;\n-\t\t\toidmap_put(&filter_data->seen_at_depth, seen_info);\n+\t\t\toidmap_put_entry(seen_info, base,\n+\t\t\t\t\t &filter_data->seen_at_depth);\n \t\t\talready_seen = 0;\n \t\t} else {\n \t\t\talready_seen =\ndiff --git a/oidmap.c b/oidmap.c\nindex 44345a8cf2..0e64c0e8a2 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -33,15 +33,18 @@ void oidmap_free(struct oidmap *map, int free_entries)\n \t\thashmap_free(&map->map);\n }\n \n-void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n+struct oidmap_entry *oidmap_get(const struct oidmap *map,\n+\t\tconst struct object_id *key)\n {\n \tif (!map->map.cmpfn)\n \t\treturn NULL;\n \n-\treturn hashmap_get_from_hash(&map->map, oidhash(key), key);\n+\treturn oidmap_entry_from_hashmap_entry(\n+\t\thashmap_get_from_hash(&map->map, oidhash(key), key));\n }\n \n-void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n+struct oidmap_entry *oidmap_remove(struct oidmap *map,\n+\t\tconst struct object_id *key)\n {\n \tstruct hashmap_entry entry;\n \n@@ -49,16 +52,16 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n \t\toidmap_init(map, 0);\n \n \thashmap_entry_init(&entry, oidhash(key));\n-\treturn hashmap_remove(&map->map, &entry, key);\n+\treturn oidmap_entry_from_hashmap_entry(\n+\t\thashmap_remove(&map->map, &entry, key));\n }\n \n-void *oidmap_put(struct oidmap *map, void *entry)\n+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry)\n {\n-\tstruct oidmap_entry *to_put = entry;\n-\n \tif (!map->map.cmpfn)\n \t\toidmap_init(map, 0);\n \n-\thashmap_entry_init(&to_put->internal_entry, oidhash(&to_put->oid));\n-\treturn hashmap_put(&map->map, &to_put->internal_entry);\n+\thashmap_entry_init(&entry->internal_entry, oidhash(&entry->oid));\n+\treturn oidmap_entry_from_hashmap_entry(\n+\t\thashmap_put(&map->map, &entry->internal_entry));\n }\ndiff --git a/oidmap.h b/oidmap.h\nindex d79b087ab0..f250f0bf87 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -46,25 +46,55 @@ void oidmap_free(struct oidmap *map, int free_entries);\n /*\n  * Returns the oidmap entry for the specified oid, or NULL if not found.\n  */\n-void *oidmap_get(const struct oidmap *map,\n+struct oidmap_entry *oidmap_get(const struct oidmap *map,\n \t\t const struct object_id *key);\n \n+/*\n+ * Returns pointer to container struct of the oidmap entry for the\n+ * specified oid, or NULL if not found.\n+ */\n+#define oidmap_get_entry(var, member, map, key) \\\n+\tvar = container_of_or_null_offset( \\\n+\t\toidmap_get(map, key), \\\n+\t\tOFFSETOF_VAR(var, member))\n /*\n  * Adds or replaces an oidmap entry.\n  *\n- * ((struct oidmap_entry *) entry)->internal_entry will be populated by this\n- * function.\n- *\n  * Returns the replaced entry, or NULL if not found (i.e. the entry was added).\n  */\n-void *oidmap_put(struct oidmap *map, void *entry);\n+struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry);\n+\n+/*\n+ * Adds or replaces an oidmap entry.\n+ *\n+ * Returns pointer to container struct of replaced entry, or NULL if\n+ * not found (i.e. the entry was added).\n+ *\n+ * The struct type of @var contains a \"struct oidmap_entry\" @member.\n+ */\n+#define oidmap_put_entry(var, member, map) \\\n+\tcontainer_of_or_null_offset( \\\n+\t\toidmap_put(map, &var->member), \\\n+\t\tOFFSETOF_VAR(var, member))\n \n /*\n  * Removes an oidmap entry matching the specified oid.\n  *\n  * Returns the removed entry, or NULL if not found.\n  */\n-void *oidmap_remove(struct oidmap *map, const struct object_id *key);\n+struct oidmap_entry *oidmap_remove(struct oidmap *map,\n+\t\tconst struct object_id *key);\n+\n+/*\n+ * Removes an oidmap entry matching the specified oid.\n+ *\n+ * Returns pointer to container struct of the removed entry, or NULL if\n+ * not found.\n+ */\n+#define oidmap_remove_entry(var, member, map, key) \\\n+\tvar = container_of_or_null_offset( \\\n+\t\toidmap_remove(map, key), \\\n+\t\tOFFSETOF_VAR(var, member))\n \n #define oidmap_entry_from_hashmap_entry(entry) \\\n \tcontainer_of_or_null(entry, struct oidmap_entry, internal_entry)\ndiff --git a/replace-object.c b/replace-object.c\nindex 7bd9aba6ee..ab0354db7a 100644\n--- a/replace-object.c\n+++ b/replace-object.c\n@@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r,\n \toidcpy(&repl_obj->replacement, oid);\n \n \t/* Register new object */\n-\tif (oidmap_put(r->objects->replace_map, repl_obj))\n+\tif (oidmap_put_entry(repl_obj, original, r->objects->replace_map))\n \t\tdie(_(\"duplicate replace ref: %s\"), refname);\n \n \treturn 0;\n@@ -73,8 +73,9 @@ const struct object_id *do_lookup_replace_object(struct repository *r,\n \n \t/* Try to recursively replace the object */\n \twhile (depth-- > 0) {\n-\t\tstruct replace_object *repl_obj =\n-\t\t\toidmap_get(r->objects->replace_map, cur);\n+\t\tstruct replace_object *repl_obj;\n+\t\toidmap_get_entry(repl_obj, original, r->objects->replace_map,\n+\t\t\t\t cur);\n \t\tif (!repl_obj)\n \t\t\treturn cur;\n \t\tcur = &repl_obj->replacement;\ndiff --git a/sequencer.c b/sequencer.c\nindex f30bb73c70..012e7865db 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4506,7 +4506,8 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \tstruct object_id dummy;\n \tint i;\n \n-\tstring_entry = oidmap_get(&state->commit2label, oid);\n+\toidmap_get_entry(string_entry, entry, &state->commit2label, oid);\n+\n \tif (string_entry)\n \t\treturn string_entry->string;\n \n@@ -4606,7 +4607,7 @@ static const char *label_oid(struct object_id *oid, const char *label,\n \n \tFLEX_ALLOC_STR(string_entry, string, label);\n \toidcpy(&string_entry->entry.oid, oid);\n-\toidmap_put(&state->commit2label, string_entry);\n+\toidmap_put_entry(string_entry, entry, &state->commit2label);\n \n \treturn string_entry->string;\n }\n@@ -4645,7 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tstruct object_id *oid = &revs->cmdline.rev[0].item->oid;\n \t\tFLEX_ALLOC_STR(entry, string, \"onto\");\n \t\toidcpy(&entry->entry.oid, oid);\n-\t\toidmap_put(&state.commit2label, entry);\n+\t\toidmap_put_entry(entry, entry, /* member name */\n+\t\t\t\t &state.commit2label);\n \n \t\tFLEX_ALLOC_STR(onto_label_entry, label, \"onto\");\n \t\thashmap_entry_init(&onto_label_entry->entry, strihash(\"onto\"));\n@@ -4689,7 +4691,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \n \t\t\tFLEX_ALLOC_STR(entry, string, buf.buf);\n \t\t\toidcpy(&entry->entry.oid, &commit->object.oid);\n-\t\t\toidmap_put(&commit2todo, entry);\n+\t\t\toidmap_put_entry(entry, entry, /* member name */\n+\t\t\t\t\t &commit2todo);\n \n \t\t\tcontinue;\n \t\t}\n@@ -4731,7 +4734,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \n \t\tFLEX_ALLOC_STR(entry, string, buf.buf);\n \t\toidcpy(&entry->entry.oid, &commit->object.oid);\n-\t\toidmap_put(&commit2todo, entry);\n+\t\toidmap_put_entry(entry, entry, /* member name */\n+\t\t\t\t &commit2todo);\n \t}\n \n \t/*\n@@ -4769,7 +4773,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tcommit = iter->item;\n \t\tif (oidset_contains(&shown, &commit->object.oid))\n \t\t\tcontinue;\n-\t\tentry = oidmap_get(&state.commit2label, &commit->object.oid);\n+\n+\t\toidmap_get_entry(entry, entry /* member name */,\n+\t\t\t\t &state.commit2label, &commit->object.oid);\n \n \t\tif (entry)\n \t\t\tstrbuf_addf(out, \"\\n%c Branch %s\\n\", comment_line_char, entry->string);\n@@ -4793,8 +4799,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\telse {\n \t\t\tconst char *to = NULL;\n \n-\t\t\tentry = oidmap_get(&state.commit2label,\n-\t\t\t\t\t   &commit->object.oid);\n+\t\t\toidmap_get_entry(entry, entry /* member name */,\n+\t\t\t\t\t &state.commit2label, &commit->object.oid);\n \t\t\tif (entry)\n \t\t\t\tto = entry->string;\n \t\t\telse if (!rebase_cousins)\n@@ -4813,11 +4819,13 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \n \t\tfor (iter2 = list; iter2; iter2 = iter2->next) {\n \t\t\tstruct object_id *oid = &iter2->item->object.oid;\n-\t\t\tentry = oidmap_get(&commit2todo, oid);\n+\t\t\toidmap_get_entry(entry, entry /* member name */,\n+\t\t\t\t\t &commit2todo, oid);\n \t\t\t/* only show if not already upstream */\n \t\t\tif (entry)\n \t\t\t\tstrbuf_addf(out, \"%s\\n\", entry->string);\n-\t\t\tentry = oidmap_get(&state.commit2label, oid);\n+\t\t\toidmap_get_entry(entry, entry /* member name */,\n+\t\t\t\t\t &state.commit2label, oid);\n \t\t\tif (entry)\n \t\t\t\tstrbuf_addf(out, \"%s %s\\n\",\n \t\t\t\t\t    cmd_label, entry->string);\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 3f599b21b8..fc19c1cb8e 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -59,7 +59,7 @@ int cmd__oidmap(int argc, const char **argv)\n \t\t\toidcpy(&entry->entry.oid, &oid);\n \n \t\t\t/* add / replace entry */\n-\t\t\tentry = oidmap_put(&map, entry);\n+\t\t\tentry = oidmap_put_entry(entry, entry, /* member name */ &map);\n \n \t\t\t/* print and free replaced entry, if any */\n \t\t\tputs(entry ? entry->name : \"NULL\");\n@@ -73,7 +73,8 @@ int cmd__oidmap(int argc, const char **argv)\n \t\t\t}\n \n \t\t\t/* lookup entry in oidmap */\n-\t\t\tentry = oidmap_get(&map, &oid);\n+\t\t\toidmap_get_entry(entry, entry /* member name */,\n+\t\t\t\t\t&map, &oid);\n \n \t\t\t/* print result */\n \t\t\tputs(entry ? entry->name : \"NULL\");\n@@ -86,7 +87,8 @@ int cmd__oidmap(int argc, const char **argv)\n \t\t\t}\n \n \t\t\t/* remove entry from oidmap */\n-\t\t\tentry = oidmap_remove(&map, &oid);\n+\t\t\toidmap_remove_entry(entry, entry, /* member name */\n+\t\t\t\t      &map, &oid);\n \n \t\t\t/* print result and free entry*/\n \t\t\tputs(entry ? entry->name : \"NULL\");\n-- \n2.26.0\n\n"}]}