diff mbox series

[1/2] oidmap: make oidmap_free independent of struct layout

Message ID 20200408040659.14511-1-abhishekkumar8222@gmail.com
State New, archived
Headers show
Series [1/2] oidmap: make oidmap_free independent of struct layout | expand

Commit Message

Abhishek Kumar April 8, 2020, 4:06 a.m. UTC
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(-)

Comments

Junio C Hamano April 8, 2020, 6:02 a.m. UTC | #1
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)
Jeff King April 8, 2020, 9:54 p.m. UTC | #2
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 mbox series

Patch

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)