* [PATCH 1/2] oidmap: make oidmap_free independent of struct layout @ 2020-04-08 4:06 Abhishek Kumar 2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar ` (4 more replies) 0 siblings, 5 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-04-08 4:06 UTC (permalink / raw) To: git c8e424c introduced hashmap_free_entries, which can free any struct pointer, regardless of the hashmap_entry field offset. oidmap does not make use of this flexibilty, hardcoding the offset to zero instead. Let's fix this by passing struct type and member to hashmap_free_entries. Additionally, removes an erroneous semi-colon at the end of hashmap_free_entries macro. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- hashmap.h | 2 +- oidmap.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/hashmap.h b/hashmap.h index 79ae9f80de..6d0a65a39f 100644 --- a/hashmap.h +++ b/hashmap.h @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset); * where @member is the hashmap_entry struct used to associate with @map */ #define hashmap_free_entries(map, type, member) \ - hashmap_free_(map, offsetof(type, member)); + hashmap_free_(map, offsetof(type, member)) /* hashmap_entry functions */ diff --git a/oidmap.c b/oidmap.c index 423aa014a3..65d63787a8 100644 --- a/oidmap.c +++ b/oidmap.c @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries) if (!map) return; - /* TODO: make oidmap itself not depend on struct layouts */ - hashmap_free_(&map->map, free_entries ? 0 : -1); + if (free_entries) + hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry); + else + hashmap_free(&map->map); } void *oidmap_get(const struct oidmap *map, const struct object_id *key) -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] oidmap: rework iterators to return typed pointer 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar @ 2020-04-08 4:06 ` Abhishek Kumar 2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano ` (3 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-04-08 4:06 UTC (permalink / raw) To: git 87571c3 modified hashmap_iter_next to return a hashmap_entry pointer instead of void pointer. However, oidmap_iter_next is unaware of the struct type containing oidmap_entry and explicilty returns a void pointer. Rework oidmap_iter_* to include struct type and return appropriate pointer. This allows for compile-time type checks. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- oidmap.h | 27 +++++++++++++++------------ t/helper/test-oidmap.c | 2 +- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/oidmap.h b/oidmap.h index c66a83ab1d..5d6b34a7ce 100644 --- a/oidmap.h +++ b/oidmap.h @@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter hashmap_iter_init(&map->map, &iter->h_iter); } -static inline void *oidmap_iter_next(struct oidmap_iter *iter) -{ - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)hashmap_iter_next(&iter->h_iter); -} +/* + * Returns the next entry, or NULL if there are no more entries. + * + * The entry is of @type (e.g. "struct foo") and has a member of type struct + * oidmap_entry. + */ +#define oidmap_iter_next(iter, type) \ + (type *) hashmap_iter_next(&(iter)->h_iter) -static inline void *oidmap_iter_first(struct oidmap *map, - struct oidmap_iter *iter) -{ - oidmap_iter_init(map, iter); - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)oidmap_iter_next(iter); -} +/* + * Returns the first entry in @map using @iter, where the entry is of @type + * (e.g. "struct foo") and has a member of type struct oidmap_entry. + */ +#define oidmap_iter_first(map, iter, type) \ + ({oidmap_iter_init(map, iter); \ + oidmap_iter_next(iter, type); }) #endif diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c index 0acf99931e..a28bf007a8 100644 --- a/t/helper/test-oidmap.c +++ b/t/helper/test-oidmap.c @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv) struct oidmap_iter iter; oidmap_iter_init(&map, &iter); - while ((entry = oidmap_iter_next(&iter))) + while ((entry = oidmap_iter_next(&iter, struct test_entry))) printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); } else { -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar 2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar @ 2020-04-08 6:02 ` Junio C Hamano 2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar ` (2 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2020-04-08 6:02 UTC (permalink / raw) To: Abhishek Kumar; +Cc: git Abhishek Kumar <abhishekkumar8222@gmail.com> writes: > c8e424c introduced hashmap_free_entries, which can free any struct > pointer, regardless of the hashmap_entry field offset. c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06) introduced hashmap_free_entries(), which ... is how we refer to existing commits and functions. > oidmap does not make use of this flexibilty, hardcoding the offset to > zero instead. Let's fix this by passing struct type and member to > hashmap_free_entries. Makes sense. > Additionally, removes an erroneous semi-colon at the end of > hashmap_free_entries macro. s/removes/remove/ Good eyes. Of course, your if/else would have broken without this fix ;-) Looking good. Thanks. > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > --- > hashmap.h | 2 +- > oidmap.c | 6 ++++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hashmap.h b/hashmap.h > index 79ae9f80de..6d0a65a39f 100644 > --- a/hashmap.h > +++ b/hashmap.h > @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset); > * where @member is the hashmap_entry struct used to associate with @map > */ > #define hashmap_free_entries(map, type, member) \ > - hashmap_free_(map, offsetof(type, member)); > + hashmap_free_(map, offsetof(type, member)) > > /* hashmap_entry functions */ > > diff --git a/oidmap.c b/oidmap.c > index 423aa014a3..65d63787a8 100644 > --- a/oidmap.c > +++ b/oidmap.c > @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries) > if (!map) > return; > > - /* TODO: make oidmap itself not depend on struct layouts */ > - hashmap_free_(&map->map, free_entries ? 0 : -1); > + if (free_entries) > + hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry); > + else > + hashmap_free(&map->map); > } > > void *oidmap_get(const struct oidmap *map, const struct object_id *key) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] oidmap: make oidmap_free independent of struct layout 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar 2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano @ 2020-04-08 7:03 ` Abhishek Kumar 2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King 2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar 4 siblings, 1 reply; 10+ messages in thread From: Abhishek Kumar @ 2020-04-08 7:03 UTC (permalink / raw) To: git c8e424c9 (hashmap: introduce hashmap_free_entries, 2019-10-06) introduced hashmap_free_entries(), which can free any struct pointer, regardless of the hashmap_entry field offset. oidmap does not make use of this flexibilty, hardcoding the offset to zero instead. Let's fix this by passing struct type and member to hashmap_free_entries(). Additionally, remove an erroneous semi-colon at the end of hashmap_free_entries() macro. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- Changes in v2: - Fix references to earlier commits. - Grammar fixes. hashmap.h | 2 +- oidmap.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/hashmap.h b/hashmap.h index 79ae9f80de..6d0a65a39f 100644 --- a/hashmap.h +++ b/hashmap.h @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset); * where @member is the hashmap_entry struct used to associate with @map */ #define hashmap_free_entries(map, type, member) \ - hashmap_free_(map, offsetof(type, member)); + hashmap_free_(map, offsetof(type, member)) /* hashmap_entry functions */ diff --git a/oidmap.c b/oidmap.c index 423aa014a3..65d63787a8 100644 --- a/oidmap.c +++ b/oidmap.c @@ -26,8 +26,10 @@ void oidmap_free(struct oidmap *map, int free_entries) if (!map) return; - /* TODO: make oidmap itself not depend on struct layouts */ - hashmap_free_(&map->map, free_entries ? 0 : -1); + if (free_entries) + hashmap_free_entries(&map->map, struct oidmap_entry, internal_entry); + else + hashmap_free(&map->map); } void *oidmap_get(const struct oidmap *map, const struct object_id *key) -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] oidmap: rework iterators to return typed pointer 2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar @ 2020-04-08 7:03 ` Abhishek Kumar 2020-04-08 22:08 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Abhishek Kumar @ 2020-04-08 7:03 UTC (permalink / raw) To: git 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified hashmap_iter_next() to return a hashmap_entry pointer instead of void pointer. However, oidmap_iter_next() is unaware of the struct type containing oidmap_entry and explicitly returns a void pointer. Rework oidmap_iter_next() to include struct type and return appropriate pointer. This allows for compile-time type checks. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- oidmap.h | 27 +++++++++++++++------------ t/helper/test-oidmap.c | 2 +- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/oidmap.h b/oidmap.h index c66a83ab1d..5d6b34a7ce 100644 --- a/oidmap.h +++ b/oidmap.h @@ -76,18 +76,21 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter hashmap_iter_init(&map->map, &iter->h_iter); } -static inline void *oidmap_iter_next(struct oidmap_iter *iter) -{ - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)hashmap_iter_next(&iter->h_iter); -} +/* + * Returns the next entry, or NULL if there are no more entries. + * + * The entry is of @type (e.g. "struct foo") and has a member of type struct + * oidmap_entry. + */ +#define oidmap_iter_next(iter, type) \ + (type *) hashmap_iter_next(&(iter)->h_iter) -static inline void *oidmap_iter_first(struct oidmap *map, - struct oidmap_iter *iter) -{ - oidmap_iter_init(map, iter); - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)oidmap_iter_next(iter); -} +/* + * Returns the first entry in @map using @iter, where the entry is of @type + * (e.g. "struct foo") and has a member of type struct oidmap_entry. + */ +#define oidmap_iter_first(map, iter, type) \ + ({oidmap_iter_init(map, iter); \ + oidmap_iter_next(iter, type); }) #endif diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c index 0acf99931e..a28bf007a8 100644 --- a/t/helper/test-oidmap.c +++ b/t/helper/test-oidmap.c @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv) struct oidmap_iter iter; oidmap_iter_init(&map, &iter); - while ((entry = oidmap_iter_next(&iter))) + while ((entry = oidmap_iter_next(&iter, struct test_entry))) printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); } else { -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] oidmap: rework iterators to return typed pointer 2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar @ 2020-04-08 22:08 ` Jeff King 0 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2020-04-08 22:08 UTC (permalink / raw) To: Abhishek Kumar; +Cc: Junio C Hamano, git On Wed, Apr 08, 2020 at 12:33:46PM +0530, Abhishek Kumar wrote: > 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified > hashmap_iter_next() to return a hashmap_entry pointer instead of void > pointer. > > However, oidmap_iter_next() is unaware of the struct type containing > oidmap_entry and explicitly returns a void pointer. > > Rework oidmap_iter_next() to include struct type and return appropriate > pointer. This allows for compile-time type checks. Yes, I think returning a pointer to an oidmap_entry makes sense. And then we get type safety, and anybody who wants embed an oidmap_entry can use container_of() to get back to their original struct. But... > +/* > + * Returns the next entry, or NULL if there are no more entries. > + * > + * The entry is of @type (e.g. "struct foo") and has a member of type struct > + * oidmap_entry. > + */ > +#define oidmap_iter_next(iter, type) \ > + (type *) hashmap_iter_next(&(iter)->h_iter) This cast is turning a hashmap_entry into whatever type the caller passed in. But it's doing it with a straight cast. We know that hashmap_entry and oidmap_entry pointers are equivalent, but we don't know where the oidmap_entry is with respect to the user's type. I think oidmap_iter_next() should continue to be a function that returns an oidmap_entry pointer (and use container_of_or_null() to get to it from the hashmap_entry, even though we know the offset is 0). And then the caller can either use container_of() to get to their original struct, or we can provide a helper macro. See the difference between hashmap_iter_first() and hashmap_iter_first_entry(). There's no hashmap_iter_next_entry(). There could be, but instead it skipped straight to hashmap_for_each_entry(), which uses an offset within the variable rather than the type. But likewise, we could add oidmap_for_each_entry() here. > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c > index 0acf99931e..a28bf007a8 100644 > --- a/t/helper/test-oidmap.c > +++ b/t/helper/test-oidmap.c > @@ -96,7 +96,7 @@ int cmd__oidmap(int argc, const char **argv) > > struct oidmap_iter iter; > oidmap_iter_init(&map, &iter); > - while ((entry = oidmap_iter_next(&iter))) > + while ((entry = oidmap_iter_next(&iter, struct test_entry))) > printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); This works because "test_entry" has the oidmap_entry at the start. But it wouldn't work with a struct where that wasn't the case, nor would it provide any compile-time safety (because of the cast). Note that if we do want to support that and get type safety (and I think it is worth doing), oidmap_get() would need similar treatment (it returns a void pointer, but it is really a pointer to an oidmap_entry). And I guess oidmap_put() and oidmap_remove(), which returns pointers to existing entries. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] oidmap: make oidmap_free independent of struct layout 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar ` (2 preceding siblings ...) 2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar @ 2020-04-08 21:54 ` Jeff King 2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar 4 siblings, 0 replies; 10+ messages in thread From: Jeff King @ 2020-04-08 21:54 UTC (permalink / raw) To: Abhishek Kumar; +Cc: git On Wed, Apr 08, 2020 at 09:36:58AM +0530, Abhishek Kumar wrote: > c8e424c introduced hashmap_free_entries, which can free any struct > pointer, regardless of the hashmap_entry field offset. > > oidmap does not make use of this flexibilty, hardcoding the offset to > zero instead. Let's fix this by passing struct type and member to > hashmap_free_entries. I'm not sure how much this improves anything. We're now telling the hashmap code how to get to our "struct oidmap_entry" pointer by using the correct offset there. But we know that offset will always be 0, because we put our internal hashmap entry at the start of oidmap_entry. What you can't do is: struct foo { int first_member; struct oidmap_entry not_the_first_member; }; and work directly with "struct foo" pointers. You have to convert oidmap_entry pointers into foo pointers with container_of() or similar. And that is true both before and after your patch. That said, I don't mind doing this as a cleanup; there's a subtle dependency on the location of internal_entry within object_entry, and this would move towards getting rid of it. -Peff ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] oidmap: make oidmap_free independent of struct layout 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar ` (3 preceding siblings ...) 2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King @ 2020-04-25 2:59 ` Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar 4 siblings, 2 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw) To: git c8e424c introduced hashmap_free_entries, which can free any struct pointer, regardless of the hashmap_entry field offset. oidmap does not make use of this flexibilty, hardcoding the offset to zero instead. Let's fix this by passing struct type and member to hashmap_free_entries. Additionally, remove an erroneous semi-colon at the end of hashmap_free_entries macro. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- hashmap.h | 2 +- oidmap.c | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hashmap.h b/hashmap.h index 79ae9f80de..6d0a65a39f 100644 --- a/hashmap.h +++ b/hashmap.h @@ -245,7 +245,7 @@ void hashmap_free_(struct hashmap *map, ssize_t offset); * where @member is the hashmap_entry struct used to associate with @map */ #define hashmap_free_entries(map, type, member) \ - hashmap_free_(map, offsetof(type, member)); + hashmap_free_(map, offsetof(type, member)) /* hashmap_entry functions */ diff --git a/oidmap.c b/oidmap.c index 423aa014a3..44345a8cf2 100644 --- a/oidmap.c +++ b/oidmap.c @@ -26,8 +26,11 @@ void oidmap_free(struct oidmap *map, int free_entries) if (!map) return; - /* TODO: make oidmap itself not depend on struct layouts */ - hashmap_free_(&map->map, free_entries ? 0 : -1); + if (free_entries) + hashmap_free_entries( + &map->map, struct oidmap_entry, internal_entry); + else + hashmap_free(&map->map); } void *oidmap_get(const struct oidmap *map, const struct object_id *key) -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] oidmap: rework iterators to return typed pointer 2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar @ 2020-04-25 2:59 ` Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar 1 sibling, 0 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw) To: git 87571c3f (hashmap: use *_entry APIs for iteration, 2019-10-06) modified hashmap_iter_next() to return a hashmap_entry pointer instead of void pointer. However, oidmap_iter_next() is unaware of the struct type containing oidmap_entry and explicitly returns a void pointer. Rewrite oidmap_iter_next() to return oidmap_entry pointer and add helper macros to return pointers to container struct. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- oidmap.h | 41 +++++++++++++++++++++++++++++++++++------ t/helper/test-oidmap.c | 7 ++++--- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/oidmap.h b/oidmap.h index c66a83ab1d..d79b087ab0 100644 --- a/oidmap.h +++ b/oidmap.h @@ -66,6 +66,8 @@ void *oidmap_put(struct oidmap *map, void *entry); */ void *oidmap_remove(struct oidmap *map, const struct object_id *key); +#define oidmap_entry_from_hashmap_entry(entry) \ + container_of_or_null(entry, struct oidmap_entry, internal_entry) struct oidmap_iter { struct hashmap_iter h_iter; @@ -76,18 +78,45 @@ static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter hashmap_iter_init(&map->map, &iter->h_iter); } -static inline void *oidmap_iter_next(struct oidmap_iter *iter) +/* Returns the next oidmap_entry, or NULL if there are no more entries. */ +static inline struct oidmap_entry *oidmap_iter_next(struct oidmap_iter *iter) { - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)hashmap_iter_next(&iter->h_iter); + return oidmap_entry_from_hashmap_entry( + hashmap_iter_next(&iter->h_iter)); } -static inline void *oidmap_iter_first(struct oidmap *map, +/* Initializes the iterator and returns the first entry, if any. */ +static inline struct oidmap_entry *oidmap_iter_first(struct oidmap *map, struct oidmap_iter *iter) { oidmap_iter_init(map, iter); - /* TODO: this API could be reworked to do compile-time type checks */ - return (void *)oidmap_iter_next(iter); + return oidmap_iter_next(iter); } +/* + * Returns the first entry in @map using @iter, where the entry if of + * @type (e.g. "struct foo") and @member is the name of the + * "struct oidmap_entry" in @type + */ +#define oidmap_iter_first_entry(map, iter, type, member) \ + container_of_or_null(oidmap_iter_first(map, iter), type, member) + +/* Internal macro for oidmap_for_each_entry */ +#define oidmap_iter_next_entry_offset(iter, offset) \ + container_of_or_null_offset(oidmap_iter_next(iter), offset) + +/* Internal macro for oidmap_for_each_entry */ +#define oidmap_iter_first_entry_offset(map, iter, offset) \ + container_of_or_null_offset(oidmap_iter_first(map, iter), offset) + +/* + * Iterate through @map using @iter, @var is a pointer to a type + * containing a @member which is a "struct oidmap_entry" + */ +#define oidmap_for_each_entry(map, iter, var, member) \ + for (var = oidmap_iter_first_entry_offset(map, iter, \ + OFFSETOF_VAR(var, member)); \ + var; \ + var = oidmap_iter_next_entry_offset(iter, \ + OFFSETOF_VAR(var, member))) #endif diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c index 0acf99931e..3f599b21b8 100644 --- a/t/helper/test-oidmap.c +++ b/t/helper/test-oidmap.c @@ -95,9 +95,10 @@ int cmd__oidmap(int argc, const char **argv) } else if (!strcmp("iterate", cmd)) { struct oidmap_iter iter; - oidmap_iter_init(&map, &iter); - while ((entry = oidmap_iter_next(&iter))) - printf("%s %s\n", oid_to_hex(&entry->entry.oid), entry->name); + struct test_entry *ent; + + oidmap_for_each_entry(&map, &iter, ent, entry) + printf("%s %s\n", oid_to_hex(&ent->entry.oid), ent->name); } else { -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] oidmap: return oidmap_entry pointers 2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar @ 2020-04-25 2:59 ` Abhishek Kumar 1 sibling, 0 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-04-25 2:59 UTC (permalink / raw) To: git oidmap_entry is usually embedded as first member of user data structures. Since oidmap is unaware of structure, it returns an void pointer for oidmap_get(), oidmap_put() and oidmap_remove(). This pointer is rather a pointer to a oidmap entry. Teach the functions to return a oidmap_entry pointer and add helper macros to return pointers to container struct. Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> --- list-objects-filter.c | 8 +++++--- oidmap.c | 21 ++++++++++++--------- oidmap.h | 42 ++++++++++++++++++++++++++++++++++++------ replace-object.c | 7 ++++--- sequencer.c | 28 ++++++++++++++++++---------- t/helper/test-oidmap.c | 8 +++++--- 6 files changed, 80 insertions(+), 34 deletions(-) diff --git a/list-objects-filter.c b/list-objects-filter.c index 1e8d4e763d..16b45d913e 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -183,13 +183,15 @@ static enum list_objects_filter_result filter_trees_depth( return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO; case LOFS_BEGIN_TREE: - seen_info = oidmap_get( - &filter_data->seen_at_depth, &obj->oid); + oidmap_get_entry(seen_info, base, &filter_data->seen_at_depth, + &obj->oid); + if (!seen_info) { seen_info = xcalloc(1, sizeof(*seen_info)); oidcpy(&seen_info->base.oid, &obj->oid); seen_info->depth = filter_data->current_depth; - oidmap_put(&filter_data->seen_at_depth, seen_info); + oidmap_put_entry(seen_info, base, + &filter_data->seen_at_depth); already_seen = 0; } else { already_seen = diff --git a/oidmap.c b/oidmap.c index 44345a8cf2..0e64c0e8a2 100644 --- a/oidmap.c +++ b/oidmap.c @@ -33,15 +33,18 @@ void oidmap_free(struct oidmap *map, int free_entries) hashmap_free(&map->map); } -void *oidmap_get(const struct oidmap *map, const struct object_id *key) +struct oidmap_entry *oidmap_get(const struct oidmap *map, + const struct object_id *key) { if (!map->map.cmpfn) return NULL; - return hashmap_get_from_hash(&map->map, oidhash(key), key); + return oidmap_entry_from_hashmap_entry( + hashmap_get_from_hash(&map->map, oidhash(key), key)); } -void *oidmap_remove(struct oidmap *map, const struct object_id *key) +struct oidmap_entry *oidmap_remove(struct oidmap *map, + const struct object_id *key) { struct hashmap_entry entry; @@ -49,16 +52,16 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key) oidmap_init(map, 0); hashmap_entry_init(&entry, oidhash(key)); - return hashmap_remove(&map->map, &entry, key); + return oidmap_entry_from_hashmap_entry( + hashmap_remove(&map->map, &entry, key)); } -void *oidmap_put(struct oidmap *map, void *entry) +struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry) { - struct oidmap_entry *to_put = entry; - if (!map->map.cmpfn) oidmap_init(map, 0); - hashmap_entry_init(&to_put->internal_entry, oidhash(&to_put->oid)); - return hashmap_put(&map->map, &to_put->internal_entry); + hashmap_entry_init(&entry->internal_entry, oidhash(&entry->oid)); + return oidmap_entry_from_hashmap_entry( + hashmap_put(&map->map, &entry->internal_entry)); } diff --git a/oidmap.h b/oidmap.h index d79b087ab0..f250f0bf87 100644 --- a/oidmap.h +++ b/oidmap.h @@ -46,25 +46,55 @@ void oidmap_free(struct oidmap *map, int free_entries); /* * Returns the oidmap entry for the specified oid, or NULL if not found. */ -void *oidmap_get(const struct oidmap *map, +struct oidmap_entry *oidmap_get(const struct oidmap *map, const struct object_id *key); +/* + * Returns pointer to container struct of the oidmap entry for the + * specified oid, or NULL if not found. + */ +#define oidmap_get_entry(var, member, map, key) \ + var = container_of_or_null_offset( \ + oidmap_get(map, key), \ + OFFSETOF_VAR(var, member)) /* * Adds or replaces an oidmap entry. * - * ((struct oidmap_entry *) entry)->internal_entry will be populated by this - * function. - * * Returns the replaced entry, or NULL if not found (i.e. the entry was added). */ -void *oidmap_put(struct oidmap *map, void *entry); +struct oidmap_entry *oidmap_put(struct oidmap *map, struct oidmap_entry *entry); + +/* + * Adds or replaces an oidmap entry. + * + * Returns pointer to container struct of replaced entry, or NULL if + * not found (i.e. the entry was added). + * + * The struct type of @var contains a "struct oidmap_entry" @member. + */ +#define oidmap_put_entry(var, member, map) \ + container_of_or_null_offset( \ + oidmap_put(map, &var->member), \ + OFFSETOF_VAR(var, member)) /* * Removes an oidmap entry matching the specified oid. * * Returns the removed entry, or NULL if not found. */ -void *oidmap_remove(struct oidmap *map, const struct object_id *key); +struct oidmap_entry *oidmap_remove(struct oidmap *map, + const struct object_id *key); + +/* + * Removes an oidmap entry matching the specified oid. + * + * Returns pointer to container struct of the removed entry, or NULL if + * not found. + */ +#define oidmap_remove_entry(var, member, map, key) \ + var = container_of_or_null_offset( \ + oidmap_remove(map, key), \ + OFFSETOF_VAR(var, member)) #define oidmap_entry_from_hashmap_entry(entry) \ container_of_or_null(entry, struct oidmap_entry, internal_entry) diff --git a/replace-object.c b/replace-object.c index 7bd9aba6ee..ab0354db7a 100644 --- a/replace-object.c +++ b/replace-object.c @@ -26,7 +26,7 @@ static int register_replace_ref(struct repository *r, oidcpy(&repl_obj->replacement, oid); /* Register new object */ - if (oidmap_put(r->objects->replace_map, repl_obj)) + if (oidmap_put_entry(repl_obj, original, r->objects->replace_map)) die(_("duplicate replace ref: %s"), refname); return 0; @@ -73,8 +73,9 @@ const struct object_id *do_lookup_replace_object(struct repository *r, /* Try to recursively replace the object */ while (depth-- > 0) { - struct replace_object *repl_obj = - oidmap_get(r->objects->replace_map, cur); + struct replace_object *repl_obj; + oidmap_get_entry(repl_obj, original, r->objects->replace_map, + cur); if (!repl_obj) return cur; cur = &repl_obj->replacement; diff --git a/sequencer.c b/sequencer.c index f30bb73c70..012e7865db 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4506,7 +4506,8 @@ static const char *label_oid(struct object_id *oid, const char *label, struct object_id dummy; int i; - string_entry = oidmap_get(&state->commit2label, oid); + oidmap_get_entry(string_entry, entry, &state->commit2label, oid); + if (string_entry) return string_entry->string; @@ -4606,7 +4607,7 @@ static const char *label_oid(struct object_id *oid, const char *label, FLEX_ALLOC_STR(string_entry, string, label); oidcpy(&string_entry->entry.oid, oid); - oidmap_put(&state->commit2label, string_entry); + oidmap_put_entry(string_entry, entry, &state->commit2label); return string_entry->string; } @@ -4645,7 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, struct object_id *oid = &revs->cmdline.rev[0].item->oid; FLEX_ALLOC_STR(entry, string, "onto"); oidcpy(&entry->entry.oid, oid); - oidmap_put(&state.commit2label, entry); + oidmap_put_entry(entry, entry, /* member name */ + &state.commit2label); FLEX_ALLOC_STR(onto_label_entry, label, "onto"); hashmap_entry_init(&onto_label_entry->entry, strihash("onto")); @@ -4689,7 +4691,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, FLEX_ALLOC_STR(entry, string, buf.buf); oidcpy(&entry->entry.oid, &commit->object.oid); - oidmap_put(&commit2todo, entry); + oidmap_put_entry(entry, entry, /* member name */ + &commit2todo); continue; } @@ -4731,7 +4734,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, FLEX_ALLOC_STR(entry, string, buf.buf); oidcpy(&entry->entry.oid, &commit->object.oid); - oidmap_put(&commit2todo, entry); + oidmap_put_entry(entry, entry, /* member name */ + &commit2todo); } /* @@ -4769,7 +4773,9 @@ static int make_script_with_merges(struct pretty_print_context *pp, commit = iter->item; if (oidset_contains(&shown, &commit->object.oid)) continue; - entry = oidmap_get(&state.commit2label, &commit->object.oid); + + oidmap_get_entry(entry, entry /* member name */, + &state.commit2label, &commit->object.oid); if (entry) strbuf_addf(out, "\n%c Branch %s\n", comment_line_char, entry->string); @@ -4793,8 +4799,8 @@ static int make_script_with_merges(struct pretty_print_context *pp, else { const char *to = NULL; - entry = oidmap_get(&state.commit2label, - &commit->object.oid); + oidmap_get_entry(entry, entry /* member name */, + &state.commit2label, &commit->object.oid); if (entry) to = entry->string; else if (!rebase_cousins) @@ -4813,11 +4819,13 @@ static int make_script_with_merges(struct pretty_print_context *pp, for (iter2 = list; iter2; iter2 = iter2->next) { struct object_id *oid = &iter2->item->object.oid; - entry = oidmap_get(&commit2todo, oid); + oidmap_get_entry(entry, entry /* member name */, + &commit2todo, oid); /* only show if not already upstream */ if (entry) strbuf_addf(out, "%s\n", entry->string); - entry = oidmap_get(&state.commit2label, oid); + oidmap_get_entry(entry, entry /* member name */, + &state.commit2label, oid); if (entry) strbuf_addf(out, "%s %s\n", cmd_label, entry->string); diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c index 3f599b21b8..fc19c1cb8e 100644 --- a/t/helper/test-oidmap.c +++ b/t/helper/test-oidmap.c @@ -59,7 +59,7 @@ int cmd__oidmap(int argc, const char **argv) oidcpy(&entry->entry.oid, &oid); /* add / replace entry */ - entry = oidmap_put(&map, entry); + entry = oidmap_put_entry(entry, entry, /* member name */ &map); /* print and free replaced entry, if any */ puts(entry ? entry->name : "NULL"); @@ -73,7 +73,8 @@ int cmd__oidmap(int argc, const char **argv) } /* lookup entry in oidmap */ - entry = oidmap_get(&map, &oid); + oidmap_get_entry(entry, entry /* member name */, + &map, &oid); /* print result */ puts(entry ? entry->name : "NULL"); @@ -86,7 +87,8 @@ int cmd__oidmap(int argc, const char **argv) } /* remove entry from oidmap */ - entry = oidmap_remove(&map, &oid); + oidmap_remove_entry(entry, entry, /* member name */ + &map, &oid); /* print result and free entry*/ puts(entry ? entry->name : "NULL"); -- 2.26.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-25 3:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-08 4:06 [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Abhishek Kumar 2020-04-08 4:06 ` [PATCH 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-08 6:02 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Junio C Hamano 2020-04-08 7:03 ` [PATCH v2 " Abhishek Kumar 2020-04-08 7:03 ` [PATCH v2 2/2] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-08 22:08 ` Jeff King 2020-04-08 21:54 ` [PATCH 1/2] oidmap: make oidmap_free independent of struct layout Jeff King 2020-04-25 2:59 ` [PATCH v3 1/3] " Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 2/3] oidmap: rework iterators to return typed pointer Abhishek Kumar 2020-04-25 2:59 ` [PATCH v3 3/3] oidmap: return oidmap_entry pointers Abhishek Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).