| Message ID | 20200408040659.14511-1-abhishekkumar8222@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] oidmap: make oidmap_free independent of struct layout | expand |
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)
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
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)
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(-)