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@xxxxxxx> wrote:
> > > On Mar 20, 2024, at 08:17, Barry Song <21cnbao@xxxxxxxxx> wrote:
> > >
> > > From: Barry Song <v-songbaohua@xxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxxxxxxx>
> > > Cc: Chris Zankel <chris@xxxxxxxxxx>
> > > Cc: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > > Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> > > Suggested-by: Max Filippov <jcmvbkbc@xxxxxxxxx>
> > > Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> > > ---
> > > 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