diff mbox series

linux/container_of.h: Add memberof()

Message ID 20230820195222.279069-1-alx@kernel.org (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series linux/container_of.h: Add memberof() | expand

Commit Message

Alejandro Colomar Aug. 20, 2023, 7:52 p.m. UTC 
From: Alejandro Colomar <alx.manpages@gmail.com>

Hi!

On 2023-08-18 10:46, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 01:28:42PM +0800, Herbert Xu wrote:
>> On Thu, Aug 17, 2023 at 04:33:17PM +0200, Lucas Segarra Fernandez wrote:
>>>
>>> +static struct pm_status_row pm_event_rows[] = {
>>> +	PM_INFO_REGSET_ENTRY32(event_log[0], EVENT0),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[1], EVENT1),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[2], EVENT2),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[3], EVENT3),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[4], EVENT4),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[5], EVENT5),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[6], EVENT6),
>>> +	PM_INFO_REGSET_ENTRY32(event_log[7], EVENT7),
>>> +};
>>> +
>>> +static_assert(ARRAY_SIZE_OF_FIELD(struct icp_qat_fw_init_admin_pm_info, event_log) ==
>>> +	      ARRAY_SIZE(pm_event_rows));

How about the following?

static_assert(ARRAY_SIZE(pm_event_rows) ==
    ARRAY_SIZE(memberof(struct icp_qat_fw_init_admin_pm_info, event_log)));

It would only need one macro addition, without significant churn.  It's
even less typing.  Below is a scissor patch with the addition of
memberof().

I tried building the kernel, and didn't see any warnings form the patch
below.

Cheers,
Alex

>>
>> Was all of that churn just for this one line?
>>
>> How about simply declaring a macro
>>
>> 	#define QAT_NUMBER_OF_PM_EVENTS 8
>>
>> and then use it for the two arrays:
>>
>> 	static struct pm_status_row pm_event_rows[QAT_NUMBER_OF_PM_EVENTS] = {
>>
>> 	__u32 event_log[QAT_NUMBER_OF_PM_EVENTS];
>>
>> What am I missing?
> 
> Splitting ARRAY_SIZE() is very beneficial on its own.
> The static assert is slightly more robust for the big code then defining
> something that at some point can be missed or miscalculated. Yet we can
> survive with a macro if you thinks it's better.
> 

-----8<------------

Many xxxof_{member,field}() macros make use of the same construction to
refer to a member of a struct without needing a variable of the
structure type.

memberof(T, m) simplifies all of those, avoids possible mistakes in
repetition, adds a meaningful name to the construction, and improves
readability by avoiding too many parentheses together.

It uses a compound literal, which should optimized out by the compiler.
It's a bit simpler to read than the dereference of a casted null
pointer, due to having less parentheses in the implementation.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Lucas Segarra Fernandez <lucas.segarra.fernandez@intel.com>
Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 include/linux/container_of.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Aug. 21, 2023, 11:18 a.m. UTC | #1 
On Sun, Aug 20, 2023 at 09:52:22PM +0200, Alejandro Colomar wrote:
> On 2023-08-18 10:46, Andy Shevchenko wrote:
> > On Fri, Aug 18, 2023 at 01:28:42PM +0800, Herbert Xu wrote:
> >> On Thu, Aug 17, 2023 at 04:33:17PM +0200, Lucas Segarra Fernandez wrote:

...

> Many xxxof_{member,field}() macros make use of the same construction to
> refer to a member of a struct without needing a variable of the
> structure type.
> 
> memberof(T, m) simplifies all of those, avoids possible mistakes in
> repetition, adds a meaningful name to the construction, and improves
> readability by avoiding too many parentheses together.
> 
> It uses a compound literal, which should optimized out by the compiler.
> It's a bit simpler to read than the dereference of a casted null
> pointer, due to having less parentheses in the implementation.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Lucas Segarra Fernandez <lucas.segarra.fernandez@intel.com>
> Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> ---
>  include/linux/container_of.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> index 713890c867be..5e762025c780 100644
> --- a/include/linux/container_of.h
> +++ b/include/linux/container_of.h
> @@ -5,7 +5,9 @@
>  #include <linux/build_bug.h>
>  #include <linux/stddef.h>
>  
> -#define typeof_member(T, m)	typeof(((T*)0)->m)
> +
> +#define memberof(T, member)  ((T){}.member)

I'm not sure. This seems to me utilization of compound literal, while above
uses direct struct member pointer calculations.

> +#define typeof_member(T, m)  typeof(memberof(T, m))
Alejandro Colomar Aug. 21, 2023, 11:23 a.m. UTC | #2 
Hi Andy,

On 2023-08-21 13:18, Andy Shevchenko wrote:
> On Sun, Aug 20, 2023 at 09:52:22PM +0200, Alejandro Colomar wrote:
>>  
>> -#define typeof_member(T, m)	typeof(((T*)0)->m)
>> +
>> +#define memberof(T, member)  ((T){}.member)
> 
> I'm not sure. This seems to me utilization of compound literal, while above
> uses direct struct member pointer calculations.

Both can be used in most cases.  The only exception is offsetof(3), where
you need the pointer calculation.  The good thing about the compound
literal is that it's farther away from causing UB, but if that's not a
concern --using sizeof() or typeof() will usually make things safe from
UB, as there's really no dereference, but just to be a little paranoic--,
I could change the definition of memberof() to use the pointer thing.

Should I send a v2 with the pointer thing?

[I'll take some time, as I need to restore my USB with keys, which just
died yesterday.  I didn't sign this email due to that.]

Cheers,
Alex

> 
>> +#define typeof_member(T, m)  typeof(memberof(T, m))
>
diff mbox series

Patch

diff --git a/include/linux/container_of.h b/include/linux/container_of.h
index 713890c867be..5e762025c780 100644
--- a/include/linux/container_of.h
+++ b/include/linux/container_of.h
@@ -5,7 +5,9 @@ 
 #include <linux/build_bug.h>
 #include <linux/stddef.h>
 
-#define typeof_member(T, m)	typeof(((T*)0)->m)
+
+#define memberof(T, member)  ((T){}.member)
+#define typeof_member(T, m)  typeof(memberof(T, m))
 
 /**
  * container_of - cast a member of a structure out to the containing structure