[10/11] introduce container_of macro
diff mbox series

Message ID 20190826024332.3403-11-e@80x24.org
State New
Headers show
Series
  • [01/11] diff: use hashmap_entry_init on moved_entry.ent
Related show

Commit Message

Eric Wong Aug. 26, 2019, 2:43 a.m. UTC
This macro is popular within the Linux kernel for supporting
intrusive data structures such as linked lists, red-black trees,
and chained hash tables while allowing the compiler to do
type checking.

I intend to use this to remove the limitation of "hashmap_entry"
being location-dependent and to allow more compile-time type
checking.

This macro already exists in our source as "list_entry" in
list.h and making "list_entry" an alias to "container_of"
as the Linux kernel has done is a possibility.

Signed-off-by: Eric Wong <e@80x24.org>
---
 git-compat-util.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Derrick Stolee Aug. 27, 2019, 2:49 p.m. UTC | #1
On 8/25/2019 10:43 PM, Eric Wong wrote:
> This macro is popular within the Linux kernel for supporting
> intrusive data structures such as linked lists, red-black trees,
> and chained hash tables while allowing the compiler to do
> type checking.
> 
> I intend to use this to remove the limitation of "hashmap_entry"
> being location-dependent and to allow more compile-time type
> checking.
> 
> This macro already exists in our source as "list_entry" in
> list.h and making "list_entry" an alias to "container_of"
> as the Linux kernel has done is a possibility.
[snip]
> +/*
> + * container_of - Get the address of an object containing a field.
> + *
> + * @ptr: pointer to the field.
> + * @type: type of the object.
> + * @member: name of the field within the object.
> + */
> +#define container_of(ptr, type, member) \
> +	((type *) ((char *)(ptr) - offsetof(type, member)))
> +
>  #endif

I think it would be good to include at least one use of this
macro in this patch. As it stands, I need to look at the next
patch to make sense of what this is doing.

It took me a little while to parse what is happening here.
'ptr' is a pointer to the generic struct (in our case,
'struct hashmap_entry *'), while 'type' is the parent type,
and 'member' is the name of the member in 'type' that is
of type typeof(*ptr).

Perhaps this is easier to grok for others than it was for me.

-Stolee
Phillip Wood Aug. 28, 2019, 9:11 a.m. UTC | #2
On 27/08/2019 15:49, Derrick Stolee wrote:
> On 8/25/2019 10:43 PM, Eric Wong wrote:
>> This macro is popular within the Linux kernel for supporting
>> intrusive data structures such as linked lists, red-black trees,
>> and chained hash tables while allowing the compiler to do
>> type checking.
>>
>> I intend to use this to remove the limitation of "hashmap_entry"
>> being location-dependent and to allow more compile-time type
>> checking.
>>
>> This macro already exists in our source as "list_entry" in
>> list.h and making "list_entry" an alias to "container_of"
>> as the Linux kernel has done is a possibility.
> [snip]
>> +/*
>> + * container_of - Get the address of an object containing a field.
>> + *
>> + * @ptr: pointer to the field.
>> + * @type: type of the object.
>> + * @member: name of the field within the object.
>> + */
>> +#define container_of(ptr, type, member) \
>> +	((type *) ((char *)(ptr) - offsetof(type, member)))
>> +
>>   #endif
> 
> I think it would be good to include at least one use of this
> macro in this patch. As it stands, I need to look at the next
> patch to make sense of what this is doing.
> 
> It took me a little while to parse what is happening here.
> 'ptr' is a pointer to the generic struct (in our case,
> 'struct hashmap_entry *'), while 'type' is the parent type,
> and 'member' is the name of the member in 'type' that is
> of type typeof(*ptr).

It took me a couple of minutes to figure it out as well. The rest of 
this patch series adds some very welcome type safety changes, at first 
sight this patch threatens to undermine that as there is no check (and 
no compiler independent way to check) that type == typeof(*ptr). It 
would also be helpful if the commit message could explain how this can 
be used to improve type safety.

Best Wishes

Phillip

> Perhaps this is easier to grok for others than it was for me.
> 
> -Stolee
>
Eric Wong Aug. 30, 2019, 7:43 p.m. UTC | #3
Derrick Stolee <stolee@gmail.com> wrote:
> On 8/25/2019 10:43 PM, Eric Wong wrote:
> > + * container_of - Get the address of an object containing a field.
> > + *
> > + * @ptr: pointer to the field.
> > + * @type: type of the object.
> > + * @member: name of the field within the object.
> > + */
> > +#define container_of(ptr, type, member) \
> > +	((type *) ((char *)(ptr) - offsetof(type, member)))
> > +
> >  #endif
> 
> I think it would be good to include at least one use of this
> macro in this patch. As it stands, I need to look at the next
> patch to make sense of what this is doing.

Yeah, I considered making list_entry an alias of this.
But I wasn't sure about including git-compat-util.h in
list.h...

> It took me a little while to parse what is happening here.
> 'ptr' is a pointer to the generic struct (in our case,
> 'struct hashmap_entry *'), while 'type' is the parent type,
> and 'member' is the name of the member in 'type' that is
> of type typeof(*ptr).
> 
> Perhaps this is easier to grok for others than it was for me.

*shrug*  I only dabble in C, but I've been using it for a good
while.  I'm probably drawn to it because I'm not a great C
programmer like having it to prevent mistakes.  I also find
open-coded linked lists (even singly-linked ones!)
hard-to-follow, so I always reach for something like urcu/list.h
or ccan/list.h.

Patch
diff mbox series

diff --git a/git-compat-util.h b/git-compat-util.h
index 83be89de0a..4cc2c8283a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1312,4 +1312,14 @@  void unleak_memory(const void *ptr, size_t len);
  */
 #include "banned.h"
 
+/*
+ * container_of - Get the address of an object containing a field.
+ *
+ * @ptr: pointer to the field.
+ * @type: type of the object.
+ * @member: name of the field within the object.
+ */
+#define container_of(ptr, type, member) \
+	((type *) ((char *)(ptr) - offsetof(type, member)))
+
 #endif