[PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

Add a description, an example and a possible workaround to the
MACRO_ARG_REUSE check.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..86545c65cf7b 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -759,6 +759,26 @@ Indentation and Line Breaks
 Macros, Attributes and Symbols
 ------------------------------
 
+  **ARG_REUSE**
+    Using the same argument multiple times in the macro definition
+    would lead to unwanted side-effects.
+
+    For example, given a `min` macro defined like::
+
+      #define min(x, y)  ((x) < (y) ? (x) : (y))
+
+    If you call it with `min(foo(x), 0)`, it would expand to::
+
+      foo(x) < 0 ? foo(x) : 0
+
+    If `foo` has side-effects or it's an expensive calculation the
+    results might not be what the user intended.
+
+    For a workaround the idea is to define local variables to hold the
+    macro's arguments. Checkout the actual implementation of `min` in
+    include/linux/minmax.h for the full implementation of the
+    workaround.
+
   **ARRAY_SIZE**
     The ARRAY_SIZE(foo) macro should be preferred over
     sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
-- 
2.30.2

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Bagas Sanjaya]

On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote:
> +  **ARG_REUSE**
> +    Using the same argument multiple times in the macro definition
> +    would lead to unwanted side-effects.
> +
> +    For example, given a `min` macro defined like::
> +
> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
> +
> +    If you call it with `min(foo(x), 0)`, it would expand to::
> +
> +      foo(x) < 0 ? foo(x) : 0
> +

Nit: literal blocks are indented three spaces relative to surrounding
paragraph.

> +    If `foo` has side-effects or it's an expensive calculation the
> +    results might not be what the user intended.
> +
> +    For a workaround the idea is to define local variables to hold the
> +    macro's arguments. Checkout the actual implementation of `min` in
> +    include/linux/minmax.h for the full implementation of the
> +    workaround.
> +

For inline code, the correct syntax is ``some text``. However, by
convention here, the backquotes aren't used where these would be
appropriate, like variable and function names.

For the last paragraph, better say "The workaround is to define local
variables to hold macro arguments. See the min macro in
include/linux/minmax.h for example.".

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Lukas Bulwahn]

On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez
<martin.fernandez@eclypsium.com> wrote:
>
> Add a description, an example and a possible workaround to the
> MACRO_ARG_REUSE check.
>
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index b52452bc2963..86545c65cf7b 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -759,6 +759,26 @@ Indentation and Line Breaks
>  Macros, Attributes and Symbols
>  ------------------------------
>
> +  **ARG_REUSE**

The name of this checkpatch type is actually "MACRO_ARG_REUSE".

> +    Using the same argument multiple times in the macro definition
> +    would lead to unwanted side-effects.

how about "... may lead to unwanted side effects"?

Rationale: it does only lead to side effects if there are multiple
computations involved.

just on spelling:
s/side-effects/side effects/

> +
> +    For example, given a `min` macro defined like::
> +
> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
> +
> +    If you call it with `min(foo(x), 0)`, it would expand to::
> +
> +      foo(x) < 0 ? foo(x) : 0
> +
> +    If `foo` has side-effects or it's an expensive calculation the
> +    results might not be what the user intended.
> +

s/side-effects/side effects/

> +    For a workaround the idea is to define local variables to hold the
> +    macro's arguments. Checkout the actual implementation of `min` in
> +    include/linux/minmax.h for the full implementation of the
> +    workaround.
> +

I ran checkpatch on all commits from v5.17..v5.18 and looked for all
check warnings with MACRO_ARG_REUSE.

There were 35 warnings in 15 commits, touching 16 different files (4
in drivers/staging, 5 in drivers/net/wireless/, 5 in
drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/).

As far as I see it from those commits, the more common way to address
this is to check that a macro is only used locally in some file and
that all uses of that macro pass a constant value as macro argument.

Maybe we add these two as equally good alternatives?

Lukas


>    **ARRAY_SIZE**
>      The ARRAY_SIZE(foo) macro should be preferred over
>      sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
> --
> 2.30.2
>

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote:
>> +  **ARG_REUSE**
>> +    Using the same argument multiple times in the macro definition
>> +    would lead to unwanted side-effects.
>> +
>> +    For example, given a `min` macro defined like::
>> +
>> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>> +
>> +    If you call it with `min(foo(x), 0)`, it would expand to::
>> +
>> +      foo(x) < 0 ? foo(x) : 0
>> +
>
> Nit: literal blocks are indented three spaces relative to surrounding
> paragraph.

I just been told that I should be using 2 (I was using 1) and the rest
of the file have 2 spaces...

>> +    If `foo` has side-effects or it's an expensive calculation the
>> +    results might not be what the user intended.
>> +
>> +    For a workaround the idea is to define local variables to hold the
>> +    macro's arguments. Checkout the actual implementation of `min` in
>> +    include/linux/minmax.h for the full implementation of the
>> +    workaround.
>> +
>
> For inline code, the correct syntax is ``some text``.

You are right, I just misleadingly reused the syntax for some other
example in the file.

> However, by
> convention here, the backquotes aren't used where these would be
> appropriate, like variable and function names.

So you are saying that for single variables and functions you don't
use double backquotes?

> For the last paragraph, better say "The workaround is to define local
> variables to hold macro arguments. See the min macro in
> include/linux/minmax.h for example.".

I like it. Thanks.

> --
> An old man doll... just what I always wanted! - Clara
>

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez
> <martin.fernandez@eclypsium.com> wrote:
>>
>> Add a description, an example and a possible workaround to the
>> MACRO_ARG_REUSE check.
>>
>> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
>> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>> ---
>>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/dev-tools/checkpatch.rst
>> b/Documentation/dev-tools/checkpatch.rst
>> index b52452bc2963..86545c65cf7b 100644
>> --- a/Documentation/dev-tools/checkpatch.rst
>> +++ b/Documentation/dev-tools/checkpatch.rst
>> @@ -759,6 +759,26 @@ Indentation and Line Breaks
>>  Macros, Attributes and Symbols
>>  ------------------------------
>>
>> +  **ARG_REUSE**
>
> The name of this checkpatch type is actually "MACRO_ARG_REUSE".

You are right.

>> +    Using the same argument multiple times in the macro definition
>> +    would lead to unwanted side-effects.
>
> how about "... may lead to unwanted side effects"?
>
> Rationale: it does only lead to side effects if there are multiple
> computations involved.

Good point.

> just on spelling:
> s/side-effects/side effects/
>
>> +
>> +    For example, given a `min` macro defined like::
>> +
>> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>> +
>> +    If you call it with `min(foo(x), 0)`, it would expand to::
>> +
>> +      foo(x) < 0 ? foo(x) : 0
>> +
>> +    If `foo` has side-effects or it's an expensive calculation the
>> +    results might not be what the user intended.
>> +
>
> s/side-effects/side effects/
>
>> +    For a workaround the idea is to define local variables to hold the
>> +    macro's arguments. Checkout the actual implementation of `min` in
>> +    include/linux/minmax.h for the full implementation of the
>> +    workaround.
>> +
>
> I ran checkpatch on all commits from v5.17..v5.18 and looked for all
> check warnings with MACRO_ARG_REUSE.
>
> There were 35 warnings in 15 commits, touching 16 different files (4
> in drivers/staging, 5 in drivers/net/wireless/, 5 in
> drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/).
>
> As far as I see it from those commits, the more common way to address
> this is to check that a macro is only used locally in some file and
> that all uses of that macro pass a constant value as macro argument.
>
> Maybe we add these two as equally good alternatives?

Yes, that's what I did on my patch that triggered this patch. But I
don't think that's a workaround. You still have the issue there, just
that the uses of the macros are "good".

I think that falls better into the "I know what I'm doing, I'm ok with
the warning" scenario, than a proper workaround.

> Lukas
>
>
>>    **ARRAY_SIZE**
>>      The ARRAY_SIZE(foo) macro should be preferred over
>>      sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
>> --
>> 2.30.2
>>
>

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Akira Yokosawa]

Hi,
Let me chime in.

On Wed, 6 Jul 2022 10:19:46 -0300, Martin Fernandez wrote:
> On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>> On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote:
>>> +  **ARG_REUSE**
>>> +    Using the same argument multiple times in the macro definition
>>> +    would lead to unwanted side-effects.
>>> +
>>> +    For example, given a `min` macro defined like::
>>> +
>>> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>>> +
>>> +    If you call it with `min(foo(x), 0)`, it would expand to::
>>> +
>>> +      foo(x) < 0 ? foo(x) : 0
>>> +
>>
>> Nit: literal blocks are indented three spaces relative to surrounding
>> paragraph.
> 
> I just been told that I should be using 2 (I was using 1) and the rest
> of the file have 2 spaces...

I think what Bagas said above is convention of Python documentation [1].
As far I see, there is no such convention in kernel documentation.
Indents of 2 spaces are fine as far as they are consistent in
related .rst files, I suppose.

[1]: https://devguide.python.org/documenting/#use-of-whitespace

> 
>>> +    If `foo` has side-effects or it's an expensive calculation the
>>> +    results might not be what the user intended.
>>> +
>>> +    For a workaround the idea is to define local variables to hold the
>>> +    macro's arguments. Checkout the actual implementation of `min` in
>>> +    include/linux/minmax.h for the full implementation of the
>>> +    workaround.
>>> +
>>
>> For inline code, the correct syntax is ``some text``.
> 
> You are right, I just misleadingly reused the syntax for some other
> example in the file.
> 
>> However, by
>> convention here, the backquotes aren't used where these would be
>> appropriate, like variable and function names.
> 
> So you are saying that for single variables and functions you don't
> use double backquotes?

If you want crossref from the functions to their kernel-doc definitions,
you can just say function() --- no double backquotes.
If you say ``function()``, crossref won't work. See [2] for such
crossrefs.

[2]: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#cross-referencing-from-restructuredtext

For simple variables, the style is up to you.  Too much double
backquotes might make the text hard to read as plain text, though.

        Thanks, Akira

> 
>> For the last paragraph, better say "The workaround is to define local
>> variables to hold macro arguments. See the min macro in
>> include/linux/minmax.h for example.".
> 
> I like it. Thanks.
> 
>> --
>> An old man doll... just what I always wanted! - Clara
>>
> 

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

On 7/6/22, Akira Yokosawa <akiyks@gmail.com> wrote:
> Hi,
> Let me chime in.
>
> On Wed, 6 Jul 2022 10:19:46 -0300, Martin Fernandez wrote:
>> On 7/6/22, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>>> On Mon, Jul 04, 2022 at 07:57:57PM -0300, Martin Fernandez wrote:
>>>> +  **ARG_REUSE**
>>>> +    Using the same argument multiple times in the macro definition
>>>> +    would lead to unwanted side-effects.
>>>> +
>>>> +    For example, given a `min` macro defined like::
>>>> +
>>>> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>>>> +
>>>> +    If you call it with `min(foo(x), 0)`, it would expand to::
>>>> +
>>>> +      foo(x) < 0 ? foo(x) : 0
>>>> +
>>>
>>> Nit: literal blocks are indented three spaces relative to surrounding
>>> paragraph.
>>
>> I just been told that I should be using 2 (I was using 1) and the rest
>> of the file have 2 spaces...
>
> I think what Bagas said above is convention of Python documentation [1].
> As far I see, there is no such convention in kernel documentation.
> Indents of 2 spaces are fine as far as they are consistent in
> related .rst files, I suppose.
>
> [1]: https://devguide.python.org/documenting/#use-of-whitespace
>
>>
>>>> +    If `foo` has side-effects or it's an expensive calculation the
>>>> +    results might not be what the user intended.
>>>> +
>>>> +    For a workaround the idea is to define local variables to hold the
>>>> +    macro's arguments. Checkout the actual implementation of `min` in
>>>> +    include/linux/minmax.h for the full implementation of the
>>>> +    workaround.
>>>> +
>>>
>>> For inline code, the correct syntax is ``some text``.
>>
>> You are right, I just misleadingly reused the syntax for some other
>> example in the file.
>>
>>> However, by
>>> convention here, the backquotes aren't used where these would be
>>> appropriate, like variable and function names.
>>
>> So you are saying that for single variables and functions you don't
>> use double backquotes?
>
> If you want crossref from the functions to their kernel-doc definitions,
> you can just say function() --- no double backquotes.
> If you say ``function()``, crossref won't work. See [2] for such
> crossrefs.
>
> [2]:
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#cross-referencing-from-restructuredtext
>
> For simple variables, the style is up to you.  Too much double
> backquotes might make the text hard to read as plain text, though.

Great! Thanks for clearing both doubts!

>         Thanks, Akira
>
>>
>>> For the last paragraph, better say "The workaround is to define local
>>> variables to hold macro arguments. See the min macro in
>>> include/linux/minmax.h for example.".
>>
>> I like it. Thanks.
>>
>>> --
>>> An old man doll... just what I always wanted! - Clara
>>>
>>
>

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Lukas Bulwahn]

On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez
<martin.fernandez@eclypsium.com> wrote:
>
> On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez
> > <martin.fernandez@eclypsium.com> wrote:
> >>
> >> Add a description, an example and a possible workaround to the
> >> MACRO_ARG_REUSE check.
> >>
> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> >> ---
> >>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
> >>  1 file changed, 20 insertions(+)
> >>
> >> diff --git a/Documentation/dev-tools/checkpatch.rst
> >> b/Documentation/dev-tools/checkpatch.rst
> >> index b52452bc2963..86545c65cf7b 100644
> >> --- a/Documentation/dev-tools/checkpatch.rst
> >> +++ b/Documentation/dev-tools/checkpatch.rst
> >> @@ -759,6 +759,26 @@ Indentation and Line Breaks
> >>  Macros, Attributes and Symbols
> >>  ------------------------------
> >>
> >> +  **ARG_REUSE**
> >
> > The name of this checkpatch type is actually "MACRO_ARG_REUSE".
>
> You are right.
>
> >> +    Using the same argument multiple times in the macro definition
> >> +    would lead to unwanted side-effects.
> >
> > how about "... may lead to unwanted side effects"?
> >
> > Rationale: it does only lead to side effects if there are multiple
> > computations involved.
>
> Good point.
>
> > just on spelling:
> > s/side-effects/side effects/
> >
> >> +
> >> +    For example, given a `min` macro defined like::
> >> +
> >> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
> >> +
> >> +    If you call it with `min(foo(x), 0)`, it would expand to::
> >> +
> >> +      foo(x) < 0 ? foo(x) : 0
> >> +
> >> +    If `foo` has side-effects or it's an expensive calculation the
> >> +    results might not be what the user intended.
> >> +
> >
> > s/side-effects/side effects/
> >
> >> +    For a workaround the idea is to define local variables to hold the
> >> +    macro's arguments. Checkout the actual implementation of `min` in
> >> +    include/linux/minmax.h for the full implementation of the
> >> +    workaround.
> >> +
> >
> > I ran checkpatch on all commits from v5.17..v5.18 and looked for all
> > check warnings with MACRO_ARG_REUSE.
> >
> > There were 35 warnings in 15 commits, touching 16 different files (4
> > in drivers/staging, 5 in drivers/net/wireless/, 5 in
> > drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/).
> >
> > As far as I see it from those commits, the more common way to address
> > this is to check that a macro is only used locally in some file and
> > that all uses of that macro pass a constant value as macro argument.
> >
> > Maybe we add these two as equally good alternatives?
>
> Yes, that's what I did on my patch that triggered this patch. But I
> don't think that's a workaround. You still have the issue there, just
> that the uses of the macros are "good".
>
> I think that falls better into the "I know what I'm doing, I'm ok with
> the warning" scenario, than a proper workaround.
>

Well, the purpose of the checkpatch documentation is to provide some
more background information on the warning (e.g., the historic
motivation, what to consider when judging its validity) and any hints
on possible resolutions. So, I would expect to see the documentation
cover explaining the most common (reusable) resolutions. A valid
argument why a check warning can be ignored falls into such a
resolution. In fact, the category "CHECK" in checkpatch.pl already
suggests that often the resolution may be to "inspect some code, but
not modify the code and then further 'ignore' the reported warning",
as some rules in checkpatch are checking something with just some
quite weak heuristics.

So, for this patch here: How about avoiding the word "workaround" and
just state these to options as resolution, e.g., a text like this:

Here are two possible options:
- Check the macro arguments of all uses of this macro to be free of
unintended side effects. Passing a constant value is usually fine, as
the compiler will use constant propagation and further optimizations
to produce acceptable code.
- If needed, define local variables in the macro to hold the macro's
argument. See the implementation of `min` in include/linux/minmax.h as
one example of this option.

What do you think?

I really appreciate you providing some documentation for this rule. I
also appreciate the rules that checkpatch.pl checks being better
explained to all of us in the kernel community. That avoids that we
all, especially newcomers, individually wonder about what checkpatch
intends to warn us about.

Lukas

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Akira Yokosawa]

Hi,
Minor nit on reST syntax.

On Mon,  4 Jul 2022 19:57:57 -0300, Martin Fernandez wrote:
> Add a description, an example and a possible workaround to the
> MACRO_ARG_REUSE check.
> 
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index b52452bc2963..86545c65cf7b 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -759,6 +759,26 @@ Indentation and Line Breaks
>  Macros, Attributes and Symbols
>  ------------------------------
>  
> +  **ARG_REUSE**> +    Using the same argument multiple times in the macro definition
> +    would lead to unwanted side-effects.

You don't need manual emphasis as above, as this list is already
in the form of so-called "Definition Lists" [1, 2].

[1]: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#definition-lists
[2]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks

Defined terms will be automatically emphasized by Sphinx and
should look in bold face in the generated HTML/PDF.
(Style of emphasis might be customized by configuration.)

It looks like there exists other similar patterns in this file
(or might as well be in other related .rst files).  I'd suggest
removing those manual emphases in a follow-up patch.

This is only a weak suggestion, and there is no urgency.
Of course, if you have a reason to do the manual emphases,
there is no need to change.

        Thanks, Akira

> +
> +    For example, given a `min` macro defined like::
> +
> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
> +
> +    If you call it with `min(foo(x), 0)`, it would expand to::
> +
> +      foo(x) < 0 ? foo(x) : 0
> +
> +    If `foo` has side-effects or it's an expensive calculation the
> +    results might not be what the user intended.
> +
> +    For a workaround the idea is to define local variables to hold the
> +    macro's arguments. Checkout the actual implementation of `min` in
> +    include/linux/minmax.h for the full implementation of the
> +    workaround.
> +
>    **ARRAY_SIZE**
>      The ARRAY_SIZE(foo) macro should be preferred over
>      sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
> -- 
> 2.30.2

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> On Wed, Jul 6, 2022 at 3:26 PM Martin Fernandez
> <martin.fernandez@eclypsium.com> wrote:
>>
>> On 7/6/22, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>> > On Tue, Jul 5, 2022 at 1:00 AM Martin Fernandez
>> > <martin.fernandez@eclypsium.com> wrote:
>> >>
>> >> Add a description, an example and a possible workaround to the
>> >> MACRO_ARG_REUSE check.
>> >>
>> >> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
>> >> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>> >> ---
>> >>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>> >>  1 file changed, 20 insertions(+)
>> >>
>> >> diff --git a/Documentation/dev-tools/checkpatch.rst
>> >> b/Documentation/dev-tools/checkpatch.rst
>> >> index b52452bc2963..86545c65cf7b 100644
>> >> --- a/Documentation/dev-tools/checkpatch.rst
>> >> +++ b/Documentation/dev-tools/checkpatch.rst
>> >> @@ -759,6 +759,26 @@ Indentation and Line Breaks
>> >>  Macros, Attributes and Symbols
>> >>  ------------------------------
>> >>
>> >> +  **ARG_REUSE**
>> >
>> > The name of this checkpatch type is actually "MACRO_ARG_REUSE".
>>
>> You are right.
>>
>> >> +    Using the same argument multiple times in the macro definition
>> >> +    would lead to unwanted side-effects.
>> >
>> > how about "... may lead to unwanted side effects"?
>> >
>> > Rationale: it does only lead to side effects if there are multiple
>> > computations involved.
>>
>> Good point.
>>
>> > just on spelling:
>> > s/side-effects/side effects/
>> >
>> >> +
>> >> +    For example, given a `min` macro defined like::
>> >> +
>> >> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>> >> +
>> >> +    If you call it with `min(foo(x), 0)`, it would expand to::
>> >> +
>> >> +      foo(x) < 0 ? foo(x) : 0
>> >> +
>> >> +    If `foo` has side-effects or it's an expensive calculation the
>> >> +    results might not be what the user intended.
>> >> +
>> >
>> > s/side-effects/side effects/
>> >
>> >> +    For a workaround the idea is to define local variables to hold
>> >> the
>> >> +    macro's arguments. Checkout the actual implementation of `min` in
>> >> +    include/linux/minmax.h for the full implementation of the
>> >> +    workaround.
>> >> +
>> >
>> > I ran checkpatch on all commits from v5.17..v5.18 and looked for all
>> > check warnings with MACRO_ARG_REUSE.
>> >
>> > There were 35 warnings in 15 commits, touching 16 different files (4
>> > in drivers/staging, 5 in drivers/net/wireless/, 5 in
>> > drivers/net/ethernet/, 1 in drivers/net/dsa/, 1 in drivers/net/can/).
>> >
>> > As far as I see it from those commits, the more common way to address
>> > this is to check that a macro is only used locally in some file and
>> > that all uses of that macro pass a constant value as macro argument.
>> >
>> > Maybe we add these two as equally good alternatives?
>>
>> Yes, that's what I did on my patch that triggered this patch. But I
>> don't think that's a workaround. You still have the issue there, just
>> that the uses of the macros are "good".
>>
>> I think that falls better into the "I know what I'm doing, I'm ok with
>> the warning" scenario, than a proper workaround.
>>
>
> Well, the purpose of the checkpatch documentation is to provide some
> more background information on the warning (e.g., the historic
> motivation, what to consider when judging its validity) and any hints
> on possible resolutions. So, I would expect to see the documentation
> cover explaining the most common (reusable) resolutions. A valid
> argument why a check warning can be ignored falls into such a
> resolution. In fact, the category "CHECK" in checkpatch.pl already
> suggests that often the resolution may be to "inspect some code, but
> not modify the code and then further 'ignore' the reported warning",
> as some rules in checkpatch are checking something with just some
> quite weak heuristics.
>
> So, for this patch here: How about avoiding the word "workaround" and
> just state these to options as resolution, e.g., a text like this:
>
> Here are two possible options:
> - Check the macro arguments of all uses of this macro to be free of
> unintended side effects. Passing a constant value is usually fine, as
> the compiler will use constant propagation and further optimizations
> to produce acceptable code.
> - If needed, define local variables in the macro to hold the macro's
> argument. See the implementation of `min` in include/linux/minmax.h as
> one example of this option.
>
> What do you think?

Yep, that's very good. I'll go with it.

> I really appreciate you providing some documentation for this rule. I
> also appreciate the rules that checkpatch.pl checks being better
> explained to all of us in the kernel community. That avoids that we
> all, especially newcomers, individually wonder about what checkpatch
> intends to warn us about.
>
> Lukas
>

Re: [PATCH v3] doc/checkpatch: Add description to MACRO_ARG_REUSE

[Martin Fernandez]

On 7/6/22, Akira Yokosawa <akiyks@gmail.com> wrote:
> Hi,
> Minor nit on reST syntax.
>
> On Mon,  4 Jul 2022 19:57:57 -0300, Martin Fernandez wrote:
>> Add a description, an example and a possible workaround to the
>> MACRO_ARG_REUSE check.
>>
>> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
>> Acked-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>> ---
>>  Documentation/dev-tools/checkpatch.rst | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/Documentation/dev-tools/checkpatch.rst
>> b/Documentation/dev-tools/checkpatch.rst
>> index b52452bc2963..86545c65cf7b 100644
>> --- a/Documentation/dev-tools/checkpatch.rst
>> +++ b/Documentation/dev-tools/checkpatch.rst
>> @@ -759,6 +759,26 @@ Indentation and Line Breaks
>>  Macros, Attributes and Symbols
>>  ------------------------------
>>
>> +  **ARG_REUSE**> +    Using the same argument multiple times in the macro
>> definition
>> +    would lead to unwanted side-effects.
>
> You don't need manual emphasis as above, as this list is already
> in the form of so-called "Definition Lists" [1, 2].
>
> [1]:
> https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#definition-lists
> [2]:
> https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#lists-and-quote-like-blocks

Thank you for the references.

> Defined terms will be automatically emphasized by Sphinx and
> should look in bold face in the generated HTML/PDF.
> (Style of emphasis might be customized by configuration.)
>
> It looks like there exists other similar patterns in this file
> (or might as well be in other related .rst files).  I'd suggest
> removing those manual emphases in a follow-up patch.
>
> This is only a weak suggestion, and there is no urgency.
> Of course, if you have a reason to do the manual emphases,
> there is no need to change.

That's interesting. Didn't really know that. I just saw this unknown
warning for me and since there were no documentation about it I
decided to quickly add it using the rest of the document as a
template. I agree that that's not a very good approach but it was very
quick :)

I'll consider checking the syntax of the whole document for further
patches, thank you for the suggestion.

>         Thanks, Akira
>
>> +
>> +    For example, given a `min` macro defined like::
>> +
>> +      #define min(x, y)  ((x) < (y) ? (x) : (y))
>> +
>> +    If you call it with `min(foo(x), 0)`, it would expand to::
>> +
>> +      foo(x) < 0 ? foo(x) : 0
>> +
>> +    If `foo` has side-effects or it's an expensive calculation the
>> +    results might not be what the user intended.
>> +
>> +    For a workaround the idea is to define local variables to hold the
>> +    macro's arguments. Checkout the actual implementation of `min` in
>> +    include/linux/minmax.h for the full implementation of the
>> +    workaround.
>> +
>>    **ARRAY_SIZE**
>>      The ARRAY_SIZE(foo) macro should be preferred over
>>      sizeof(foo)/sizeof(foo[0]) for finding number of elements in an
>> --
>> 2.30.2
>