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@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.

> 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
> >
>
>