[PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
and loongarch,
   In file included from crypto/scompress.c:12:
   include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
   include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
      76 |                 struct page *page;
         |                              ^~~~
   crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
     174 |                         struct page *dst_page = sg_page(req->dst);
         |

The reason is that flush_dcache_page() is implemented as a noop
macro on these platforms as below,

 #define flush_dcache_page(page) do { } while (0)

The driver code, for itself, seems be quite innocent and placing
maybe_unused seems pointless,

 struct page *dst_page = sg_page(req->dst);

 for (i = 0; i < nr_pages; i++)
 	flush_dcache_page(dst_page + i);

And it should be independent of architectural implementation
differences.

Let's have a guidance in codingstyle to ask for the evaluation
of parameters.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Zankel <chris@zankel.net>
Cc: Huacai Chen <chenhuacai@loongson.cn>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 Documentation/process/coding-style.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 9c7cf7347394..8065747fddff 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
 				do_this(b, c);		\
 		} while (0)
 
+Function-like macros should evaluate their parameters, for unused parameters,
+cast them to void:
+
+.. code-block:: c
+
+	#define macrofun(a) do { (void) (a); } while (0)
+
 Things to avoid when using macros:
 
 1) macros that affect control flow:
-- 
2.34.1

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

Hi Barry,

On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <21cnbao@gmail.com> wrote:
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 9c7cf7347394..8065747fddff 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
>  				do_this(b, c);		\
>  		} while (0)
>  
> +Function-like macros should evaluate their parameters, for unused parameters,
> +cast them to void:
> +
> +.. code-block:: c
> +
> +	#define macrofun(a) do { (void) (a); } while (0)
> +

Maybe add some comment about using a static inline function for these
simple versions instead, if at all possible, (it is suggested just
above this section) since that will still type check arguments.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

Hi Stephen,
Thanks for reviewing.

On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Barry,
>
> On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <21cnbao@gmail.com> wrote:
> >
> > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > index 9c7cf7347394..8065747fddff 100644
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> >                               do_this(b, c);          \
> >               } while (0)
> >
> > +Function-like macros should evaluate their parameters, for unused parameters,
> > +cast them to void:
> > +
> > +.. code-block:: c
> > +
> > +     #define macrofun(a) do { (void) (a); } while (0)
> > +
>
> Maybe add some comment about using a static inline function for these
> simple versions instead, if at all possible, (it is suggested just
> above this section) since that will still type check arguments.

right, what about adding the below section together with the above (void) cast?

+Another approach could involve utilizing a static inline function to replace
+the macro.:
+
+.. code-block:: c
+
+       static inline void fun(struct foo *foo)
+       {
+       }
+

>
> --
> Cheers,
> Stephen Rothwell

Thanks
Barry

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

Hi Barry,

On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <21cnbao@gmail.com> wrote:
>
> On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <21cnbao@gmail.com> wrote:  
> > >
> > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > > index 9c7cf7347394..8065747fddff 100644
> > > --- a/Documentation/process/coding-style.rst
> > > +++ b/Documentation/process/coding-style.rst
> > > @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> > >                               do_this(b, c);          \
> > >               } while (0)
> > >
> > > +Function-like macros should evaluate their parameters, for unused parameters,
> > > +cast them to void:
> > > +
> > > +.. code-block:: c
> > > +
> > > +     #define macrofun(a) do { (void) (a); } while (0)
> > > +  
> >
> > Maybe add some comment about using a static inline function for these
> > simple versions instead, if at all possible, (it is suggested just
> > above this section) since that will still type check arguments.  
> 
> right, what about adding the below section together with the above (void) cast?
> 
> +Another approach could involve utilizing a static inline function to replace
> +the macro.:
> +
> +.. code-block:: c
> +
> +       static inline void fun(struct foo *foo)
> +       {
> +       }
> +

Looks good to me.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Andrew Morton]

On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <21cnbao@gmail.com> wrote:

> Hi Stephen,
> Thanks for reviewing.
> 
> On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > Hi Barry,
> >
> > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > > index 9c7cf7347394..8065747fddff 100644
> > > --- a/Documentation/process/coding-style.rst
> > > +++ b/Documentation/process/coding-style.rst
> > > @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> > >                               do_this(b, c);          \
> > >               } while (0)
> > >
> > > +Function-like macros should evaluate their parameters, for unused parameters,
> > > +cast them to void:
> > > +
> > > +.. code-block:: c
> > > +
> > > +     #define macrofun(a) do { (void) (a); } while (0)
> > > +
> >
> > Maybe add some comment about using a static inline function for these
> > simple versions instead, if at all possible, (it is suggested just
> > above this section) since that will still type check arguments.
> 
> right, what about adding the below section together with the above (void) cast?
> 
> +Another approach could involve utilizing a static inline function to replace
> +the macro.:
> +
> +.. code-block:: c
> +
> +       static inline void fun(struct foo *foo)
> +       {
> +       }
> +

Stronger than that please.  Just tell people not to use macros in such
situations.  Always code it in C.

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

On Thu, Mar 21, 2024 at 4:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <21cnbao@gmail.com> wrote:
>
> > Hi Stephen,
> > Thanks for reviewing.
> >
> > On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > >
> > > Hi Barry,
> > >
> > > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <21cnbao@gmail.com> wrote:
> > > >
> > > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > > > index 9c7cf7347394..8065747fddff 100644
> > > > --- a/Documentation/process/coding-style.rst
> > > > +++ b/Documentation/process/coding-style.rst
> > > > @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> > > >                               do_this(b, c);          \
> > > >               } while (0)
> > > >
> > > > +Function-like macros should evaluate their parameters, for unused parameters,
> > > > +cast them to void:
> > > > +
> > > > +.. code-block:: c
> > > > +
> > > > +     #define macrofun(a) do { (void) (a); } while (0)
> > > > +
> > >
> > > Maybe add some comment about using a static inline function for these
> > > simple versions instead, if at all possible, (it is suggested just
> > > above this section) since that will still type check arguments.
> >
> > right, what about adding the below section together with the above (void) cast?
> >
> > +Another approach could involve utilizing a static inline function to replace
> > +the macro.:
> > +
> > +.. code-block:: c
> > +
> > +       static inline void fun(struct foo *foo)
> > +       {
> > +       }
> > +
>
> Stronger than that please.  Just tell people not to use macros in such
> situations.  Always code it in C.

While I appreciate the consistency of always using "static inline"
instead of macros,
I've noticed numerous instances of (void) macros throughout the kernel.

arch/arm64/include/asm/cpuidle.h:#define arm_cpuidle_save_irq_context(c) (void)c
arch/arm64/include/asm/cpuidle.h:#define
arm_cpuidle_restore_irq_context(c) (void)c
arch/loongarch/include/asm/io.h:#define iounmap(addr) ((void)(addr))
arch/mips/include/asm/cop2.h:#define cop2_save(r) do { (void)(r); } while (0)
arch/mips/include/asm/cop2.h:#define cop2_restore(r) do { (void)(r); } while (0)
arch/mips/include/asm/cop2.h:#define cop2_save(r) do { (void)(r); } while (0)
arch/mips/include/asm/cop2.h:#define cop2_restore(r) do { (void)(r); } while (0)
....

I'm uncertain whether people would find it disconcerting if they completely
deviate from the current approach.

If you believe it won't pose an issue, I can proceed with v3 to eliminate
the first option, casting to (void).

Thanks
Barry

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Meiyong Yu]

> On Mar 20, 2024, at 08:17, Barry Song <21cnbao@gmail.com> wrote:
> 
> From: Barry Song <v-songbaohua@oppo.com>
> 
> Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
> sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
> and loongarch,
>   In file included from crypto/scompress.c:12:
>   include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
>   include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
>      76 |                 struct page *page;
>         |                              ^~~~
>   crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
>     174 |                         struct page *dst_page = sg_page(req->dst);
>         |
> 
> The reason is that flush_dcache_page() is implemented as a noop
> macro on these platforms as below,
> 
> #define flush_dcache_page(page) do { } while (0)
> 
> The driver code, for itself, seems be quite innocent and placing
> maybe_unused seems pointless,
> 
> struct page *dst_page = sg_page(req->dst);
> 
> for (i = 0; i < nr_pages; i++)
>    flush_dcache_page(dst_page + i);
> 
> And it should be independent of architectural implementation
> differences.
> 
> Let's have a guidance in codingstyle to ask for the evaluation
> of parameters.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Huacai Chen <chenhuacai@loongson.cn>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
> Documentation/process/coding-style.rst | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 9c7cf7347394..8065747fddff 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
>                do_this(b, c);        \
>        } while (0)
> 


> +Function-like macros should evaluate their parameters, for unused parameters,
I do not support this point, if the parameter is unused, why not to remove it.

about the warning,  is  tool misreport,  the tool must make better

> +cast them to void:
> +
> +.. code-block:: c
> +
> +    #define macrofun(a) do { (void) (a); } while (0)
> +
> Things to avoid when using macros:
> 
> 1) macros that affect control flow:
> --
> 2.34.1
> 

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <meiyong.yu@126.com> wrote:
>
>
> > On Mar 20, 2024, at 08:17, Barry Song <21cnbao@gmail.com> wrote:
> >
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
> > sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
> > and loongarch,
> >   In file included from crypto/scompress.c:12:
> >   include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> >   include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> >      76 |                 struct page *page;
> >         |                              ^~~~
> >   crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> >     174 |                         struct page *dst_page = sg_page(req->dst);
> >         |
> >
> > The reason is that flush_dcache_page() is implemented as a noop
> > macro on these platforms as below,
> >
> > #define flush_dcache_page(page) do { } while (0)
> >
> > The driver code, for itself, seems be quite innocent and placing
> > maybe_unused seems pointless,
> >
> > struct page *dst_page = sg_page(req->dst);
> >
> > for (i = 0; i < nr_pages; i++)
> >    flush_dcache_page(dst_page + i);
> >
> > And it should be independent of architectural implementation
> > differences.
> >
> > Let's have a guidance in codingstyle to ask for the evaluation
> > of parameters.
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Chris Zankel <chris@zankel.net>
> > Cc: Huacai Chen <chenhuacai@loongson.cn>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > ---
> > Documentation/process/coding-style.rst | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > index 9c7cf7347394..8065747fddff 100644
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> >                do_this(b, c);        \
> >        } while (0)
> >
>
>
> > +Function-like macros should evaluate their parameters, for unused parameters,
> I do not support this point, if the parameter is unused, why not to remove it.
>

Linux boasts support for numerous architectures, striving for
independence in its
drivers and core code implementation across these architectures. Consequently,
certain architectures may utilize parameters for the same APIs, while others may
not.

> about the warning,  is  tool misreport,  the tool must make better
>

no. This is not the case.

> > +cast them to void:
> > +
> > +.. code-block:: c
> > +
> > +    #define macrofun(a) do { (void) (a); } while (0)
> > +
> > Things to avoid when using macros:
> >
> > 1) macros that affect control flow:
> > --
> > 2.34.1
> >
>
>

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Meiyong Yu]

在 2024/3/21 8:11, Barry Song 写道:
> On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <meiyong.yu@126.com> wrote:
>>
>>> On Mar 20, 2024, at 08:17, Barry Song <21cnbao@gmail.com> wrote:
>>>
>>> From: Barry Song <v-songbaohua@oppo.com>
>>>
>>> Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
>>> sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
>>> and loongarch,
>>>    In file included from crypto/scompress.c:12:
>>>    include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
>>>    include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
>>>       76 |                 struct page *page;
>>>          |                              ^~~~
>>>    crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
>>>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
>>>      174 |                         struct page *dst_page = sg_page(req->dst);
>>>          |
>>>
>>> The reason is that flush_dcache_page() is implemented as a noop
>>> macro on these platforms as below,
>>>
>>> #define flush_dcache_page(page) do { } while (0)
>>>
>>> The driver code, for itself, seems be quite innocent and placing
>>> maybe_unused seems pointless,
>>>
>>> struct page *dst_page = sg_page(req->dst);
>>>
>>> for (i = 0; i < nr_pages; i++)
>>>     flush_dcache_page(dst_page + i);
>>>
>>> And it should be independent of architectural implementation
>>> differences.
>>>
>>> Let's have a guidance in codingstyle to ask for the evaluation
>>> of parameters.
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Chris Zankel <chris@zankel.net>
>>> Cc: Huacai Chen <chenhuacai@loongson.cn>
>>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>> ---
>>> Documentation/process/coding-style.rst | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
>>> index 9c7cf7347394..8065747fddff 100644
>>> --- a/Documentation/process/coding-style.rst
>>> +++ b/Documentation/process/coding-style.rst
>>> @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
>>>                 do_this(b, c);        \
>>>         } while (0)
>>>
>>
>>> +Function-like macros should evaluate their parameters, for unused parameters,
>> I do not support this point, if the parameter is unused, why not to remove it.
>>
> Linux boasts support for numerous architectures, striving for
> independence in its
> drivers and core code implementation across these architectures. Consequently,
> certain architectures may utilize parameters for the same APIs, while others may
> not.

So the probem is  designed api is not reasonable,  it use not essential 
paramter,

you can change the api, but not avoid it.

Anthor question, why you do not use the parameter, if not use it,  will 
trigger function/feature dismiss problem ?

>> about the warning,  is  tool misreport,  the tool must make better
>>
> no. This is not the case.
>
>>> +cast them to void:
>>> +
>>> +.. code-block:: c
>>> +
>>> +    #define macrofun(a) do { (void) (a); } while (0)
>>> +
>>> Things to avoid when using macros:
>>>
>>> 1) macros that affect control flow:
>>> --
>>> 2.34.1
>>>
>>

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

On Thu, Mar 21, 2024 at 5:40 PM Meiyong Yu <meiyong.yu@126.com> wrote:
>
>
> 在 2024/3/21 8:11, Barry Song 写道:
> > On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <meiyong.yu@126.com> wrote:
> >>
> >>> On Mar 20, 2024, at 08:17, Barry Song <21cnbao@gmail.com> wrote:
> >>>
> >>> From: Barry Song <v-songbaohua@oppo.com>
> >>>
> >>> Recent commit 77292bb8ca69c80 ("crypto: scomp - remove memcpy if
> >>> sg_nents is 1 and pages are lowmem") leads to warnings on xtensa
> >>> and loongarch,
> >>>    In file included from crypto/scompress.c:12:
> >>>    include/crypto/scatterwalk.h: In function 'scatterwalk_pagedone':
> >>>    include/crypto/scatterwalk.h:76:30: warning: variable 'page' set but not used [-Wunused-but-set-variable]
> >>>       76 |                 struct page *page;
> >>>          |                              ^~~~
> >>>    crypto/scompress.c: In function 'scomp_acomp_comp_decomp':
> >>>>> crypto/scompress.c:174:38: warning: unused variable 'dst_page' [-Wunused-variable]
> >>>      174 |                         struct page *dst_page = sg_page(req->dst);
> >>>          |
> >>>
> >>> The reason is that flush_dcache_page() is implemented as a noop
> >>> macro on these platforms as below,
> >>>
> >>> #define flush_dcache_page(page) do { } while (0)
> >>>
> >>> The driver code, for itself, seems be quite innocent and placing
> >>> maybe_unused seems pointless,
> >>>
> >>> struct page *dst_page = sg_page(req->dst);
> >>>
> >>> for (i = 0; i < nr_pages; i++)
> >>>     flush_dcache_page(dst_page + i);
> >>>
> >>> And it should be independent of architectural implementation
> >>> differences.
> >>>
> >>> Let's have a guidance in codingstyle to ask for the evaluation
> >>> of parameters.
> >>>
> >>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>> Cc: Chris Zankel <chris@zankel.net>
> >>> Cc: Huacai Chen <chenhuacai@loongson.cn>
> >>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >>> Cc: Guenter Roeck <linux@roeck-us.net>
> >>> Suggested-by: Max Filippov <jcmvbkbc@gmail.com>
> >>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >>> ---
> >>> Documentation/process/coding-style.rst | 7 +++++++
> >>> 1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> >>> index 9c7cf7347394..8065747fddff 100644
> >>> --- a/Documentation/process/coding-style.rst
> >>> +++ b/Documentation/process/coding-style.rst
> >>> @@ -827,6 +827,13 @@ Macros with multiple statements should be enclosed in a do - while block:
> >>>                 do_this(b, c);        \
> >>>         } while (0)
> >>>
> >>
> >>> +Function-like macros should evaluate their parameters, for unused parameters,
> >> I do not support this point, if the parameter is unused, why not to remove it.
> >>
> > Linux boasts support for numerous architectures, striving for
> > independence in its
> > drivers and core code implementation across these architectures. Consequently,
> > certain architectures may utilize parameters for the same APIs, while others may
> > not.
>
> So the probem is  designed api is not reasonable,  it use not essential
> paramter,
>
> you can change the api, but not avoid it.
>

Incorrect again. As an API, it must take into account various considerations.
Just because architecture A doesn't require flushing dcache doesn't imply
that architecture B doesn't need it.

> Anthor question, why you do not use the parameter, if not use it,  will
> trigger function/feature dismiss problem ?
>
> >> about the warning,  is  tool misreport,  the tool must make better
> >>
> > no. This is not the case.
> >
> >>> +cast them to void:
> >>> +
> >>> +.. code-block:: c
> >>> +
> >>> +    #define macrofun(a) do { (void) (a); } while (0)
> >>> +
> >>> Things to avoid when using macros:
> >>>
> >>> 1) macros that affect control flow:
> >>> --
> >>> 2.34.1
> >>>
> >>
>
>

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On Thu, Mar 21, 2024 at 07:48:36AM +1300, Barry Song wrote:
> On Thu, Mar 21, 2024 at 4:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:

> > Stronger than that please.  Just tell people not to use macros in such
> > situations.  Always code it in C.

> While I appreciate the consistency of always using "static inline"
> instead of macros,
> I've noticed numerous instances of (void) macros throughout the kernel.

...

> I'm uncertain whether people would find it disconcerting if they completely
> deviate from the current approach.

> If you believe it won't pose an issue, I can proceed with v3 to eliminate
> the first option, casting to (void).

It might be worth adding a note somewhere in the file that talks about
how the coding style document is convering the current state of the art
but some files might older and not following the current style.  This
isn't going to be the only thing where there'll be issues like this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Andrew Morton]

On Thu, 21 Mar 2024 07:48:36 +1300 Barry Song <21cnbao@gmail.com> wrote:

> > Stronger than that please.  Just tell people not to use macros in such
> > situations.  Always code it in C.
> 
> While I appreciate the consistency of always using "static inline"
> instead of macros,
> I've noticed numerous instances of (void) macros throughout the kernel.
> 
> arch/arm64/include/asm/cpuidle.h:#define arm_cpuidle_save_irq_context(c) (void)c
> arch/arm64/include/asm/cpuidle.h:#define
> arm_cpuidle_restore_irq_context(c) (void)c
> arch/loongarch/include/asm/io.h:#define iounmap(addr) ((void)(addr))
> arch/mips/include/asm/cop2.h:#define cop2_save(r) do { (void)(r); } while (0)
> arch/mips/include/asm/cop2.h:#define cop2_restore(r) do { (void)(r); } while (0)
> arch/mips/include/asm/cop2.h:#define cop2_save(r) do { (void)(r); } while (0)
> arch/mips/include/asm/cop2.h:#define cop2_restore(r) do { (void)(r); } while (0)
> ....
> 
> I'm uncertain whether people would find it disconcerting if they completely
> deviate from the current approach.
> 
> If you believe it won't pose an issue, I can proceed with v3 to eliminate
> the first option, casting to (void).

I think so.  My overall view is that we should write things in C.  Only
use macros if the thing we're trying to do simply cannot be done in a C
function.

- inline functions don't have the "expression with side effects
  evaluated more than once" problem.

- inline functions avoid the unused-variable issue which started this thread

- inline functions look better

- for some reason, people are more inclined to document inline
  functions than macros.

Re: [PATCH] Documentation: coding-style: ask function-like macros to evaluate parameters

[Barry Song]

On Fri, Mar 22, 2024 at 12:15 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Mar 21, 2024 at 07:48:36AM +1300, Barry Song wrote:
> > On Thu, Mar 21, 2024 at 4:49 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > > Stronger than that please.  Just tell people not to use macros in such
> > > situations.  Always code it in C.
>
> > While I appreciate the consistency of always using "static inline"
> > instead of macros,
> > I've noticed numerous instances of (void) macros throughout the kernel.
>
> ...
>
> > I'm uncertain whether people would find it disconcerting if they completely
> > deviate from the current approach.
>
> > If you believe it won't pose an issue, I can proceed with v3 to eliminate
> > the first option, casting to (void).
>
> It might be worth adding a note somewhere in the file that talks about
> how the coding style document is convering the current state of the art
> but some files might older and not following the current style.  This
> isn't going to be the only thing where there'll be issues like this.


I'm not entirely sure where to add the comment, but at least I can address
this specific case by rewriting it as follows:

diff --git a/Documentation/process/coding-style.rst
b/Documentation/process/coding-style.rst
index 9c7cf7347394..791d333a57fd 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -827,6 +827,22 @@ Macros with multiple statements should be
enclosed in a do - while block:
                                do_this(b, c);          \
                } while (0)

+Function-like macros with unused parameters should be replaced by static
+inline functions to avoid the issue of unused variables:
+
+.. code-block:: c
+
+       static inline void fun(struct foo *foo)
+       {
+       }
+
+For historical reasons, many files still use the cast to (void) to evaluate
+parameters, but this method is not recommended:
+
+.. code-block:: c
+
+       #define macrofun(foo) do { (void) (foo); } while (0)
+
 Things to avoid when using macros:

 1) macros that affect control flow:


Mark, Andrew,
Does it make sense to you?

Thanks
Barry