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

[Martin Fernandez <martin.fernandez@eclypsium.com>]

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
>