| Message ID | 20190826024332.3403-11-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] diff: use hashmap_entry_init on moved_entry.ent | expand |
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
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 >
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.
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
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(+)