From: Barry Song <[email protected]>
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 <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: Herbert Xu <[email protected]>
Cc: Guenter Roeck <[email protected]>
Suggested-by: Max Filippov <[email protected]>
Signed-off-by: Barry Song <[email protected]>
---
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
Hi Barry,
On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <[email protected]> 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
Hi Stephen,
Thanks for reviewing.
On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <[email protected]> wrote:
>
> Hi Barry,
>
> On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <[email protected]> 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
Hi Barry,
On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <[email protected]> wrote:
> >
> > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <[email protected]> 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
On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <[email protected]> wrote:
> Hi Stephen,
> Thanks for reviewing.
>
> On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <[email protected]> wrote:
> >
> > Hi Barry,
> >
> > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <[email protected]> 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.
On Thu, Mar 21, 2024 at 4:49 AM Andrew Morton <[email protected]> wrote:
>
> On Wed, 20 Mar 2024 16:24:30 +1300 Barry Song <[email protected]> wrote:
>
> > Hi Stephen,
> > Thanks for reviewing.
> >
> > On Wed, Mar 20, 2024 at 2:42 PM Stephen Rothwell <[email protected]> wrote:
> > >
> > > Hi Barry,
> > >
> > > On Wed, 20 Mar 2024 13:16:56 +1300 Barry Song <[email protected]> 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
> On Mar 20, 2024, at 08:17, Barry Song <[email protected]> wrote:
>
> From: Barry Song <[email protected]>
>
> 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 <[email protected]>
> Cc: Chris Zankel <[email protected]>
> Cc: Huacai Chen <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Suggested-by: Max Filippov <[email protected]>
> Signed-off-by: Barry Song <[email protected]>
> ---
> 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
>
On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <[email protected]> wrote:
>
>
> > On Mar 20, 2024, at 08:17, Barry Song <[email protected]> wrote:
> >
> > From: Barry Song <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Chris Zankel <[email protected]>
> > Cc: Huacai Chen <[email protected]>
> > Cc: Herbert Xu <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Suggested-by: Max Filippov <[email protected]>
> > Signed-off-by: Barry Song <[email protected]>
> > ---
> > 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
> >
>
>
在 2024/3/21 8:11, Barry Song 写道:
> On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <[email protected]> wrote:
>>
>>> On Mar 20, 2024, at 08:17, Barry Song <[email protected]> wrote:
>>>
>>> From: Barry Song <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Cc: Chris Zankel <[email protected]>
>>> Cc: Huacai Chen <[email protected]>
>>> Cc: Herbert Xu <[email protected]>
>>> Cc: Guenter Roeck <[email protected]>
>>> Suggested-by: Max Filippov <[email protected]>
>>> Signed-off-by: Barry Song <[email protected]>
>>> ---
>>> 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
>>>
>>
On Thu, Mar 21, 2024 at 5:40 PM Meiyong Yu <[email protected]> wrote:
>
>
> 在 2024/3/21 8:11, Barry Song 写道:
> > On Thu, Mar 21, 2024 at 12:39 PM Meiyong Yu <[email protected]> wrote:
> >>
> >>> On Mar 20, 2024, at 08:17, Barry Song <[email protected]> wrote:
> >>>
> >>> From: Barry Song <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> Cc: Chris Zankel <[email protected]>
> >>> Cc: Huacai Chen <[email protected]>
> >>> Cc: Herbert Xu <[email protected]>
> >>> Cc: Guenter Roeck <[email protected]>
> >>> Suggested-by: Max Filippov <[email protected]>
> >>> Signed-off-by: Barry Song <[email protected]>
> >>> ---
> >>> 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
> >>>
> >>
>
>
On Thu, Mar 21, 2024 at 07:48:36AM +1300, Barry Song wrote:
> On Thu, Mar 21, 2024 at 4:49 AM Andrew Morton <[email protected]> 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.
On Thu, 21 Mar 2024 07:48:36 +1300 Barry Song <[email protected]> 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.
On Fri, Mar 22, 2024 at 12:15 AM Mark Brown <[email protected]> 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 <[email protected]> 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