Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a6fc8ce8-4fe1-7bf6-c92d-b793d24e74a1@efficios.com>
Date:   Mon, 5 Jun 2023 11:31:21 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [RFC PATCH] Documentation: Document macro coding style
Content-Language: en-US
To:     Jonathan Corbet <corbet@lwn.net>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc:     linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        linux-doc@vger.kernel.org
References: <20230511152951.1970870-1-mathieu.desnoyers@efficios.com>
 <87y1lkmjjv.fsf@meer.lwn.net>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <87y1lkmjjv.fsf@meer.lwn.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/19/23 11:23, Jonathan Corbet wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
>> Document the kernel coding style for macros with parameters.
>>
>> The purpose of this text is to be used as a reference to gradually
>> transition towards macros with a more consistent style, and eliminate
>> subtle bugs that can creep up due to missing parentheses, and generally
>> remove the need to think and argue about C operator precedence.
>>
>> This is based on a mailing list discussion with Linus.
>>
>> Link: https://lore.kernel.org/lkml/CAHk-=wjfgCa-u8h9z+8U7gaKK6PnRCpws1Md9wYSSXywUxoUSA@mail.gmail.com/
>> Link: https://lore.kernel.org/lkml/CAHk-=wjzpHjqhybyEhkTzGgTdBP3LZ1FmOw8=1MMXr=-j5OPxQ@mail.gmail.com/
>> Link: https://lore.kernel.org/lkml/CAHk-=wh-x1PL=UUGD__Dv6kd+kyCHjNF-TCHGG9ayLnysf-PdQ@mail.gmail.com/
>> Link: https://lore.kernel.org/lkml/CAHk-=wg27iiFZWYmjKmULxwkXisOHuAXq=vbiazBabgh9M1rqg@mail.gmail.com/
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: linux-doc@vger.kernel.org
>> ---
>>   Documentation/process/coding-style.rst | 152 ++++++++++++++++++++++++-
>>   1 file changed, 151 insertions(+), 1 deletion(-)
> 
> So this looks generally OK to me.  I really like to see some reviews /
> acks on coding-style patches, though; I don't feel like I should be the
> arbiter of kernel coding style.

Sure, I'll update the patch based on your comments and send without the RFC tag, we'll
see if we get reviews or ack.

Meanwhile there are two additional cases where I think we should mandate the parentheses
even though they are not strictly needed, because it eliminates corner-cases without
negatively impacting readability.

The first case is when an argument is used as declarator identifier, e.g.

         #define DECLARE_WAITQUEUE(name, tsk)    \
                 struct wait_queue_entry name = __WAITQUEUE_INITIALIZER(name, tsk)
                                         ^ here

Adding parentheses around "name" does not break anything, makes the code *more*
regular by following the general rule of adding parentheses around macro arguments
unless it is not possible for some reason.

I also think that we should mandate parentheses around initializers, even though
those are full expressions:

         #define foo(a)                                  \
                 do {                                    \
                         int __m_var = a;                \
                                       ^ here
                 } while (0)

Because requiring the (useless) parentheses here makes the code more consistent and
removes a corner-case to think about when writing code, without negatively impacting
readability.

This would basically leave only the following corner-cases. Please let me know if any
of them would be good candidates for requiring the extra parentheses nevertheless:

- For readability:

   - "a;"
   - "for (a; b; c)"
   - "if (a)"
   - "switch (a)"
   - "while (a)"
   - "do { ... } while (a)"
   - "return a;"
   - "array[a]",
   - "f(a)",
   - "f(a, b, c)",

Because it would not work otherwise:

- "(p)->a,
- "#a",
- "sym##a".

> 
> One little comment below
> 
>> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
>> index 6db37a46d305..3cf62c91d91c 100644
>> --- a/Documentation/process/coding-style.rst
>> +++ b/Documentation/process/coding-style.rst
>> @@ -819,10 +819,160 @@ Macros with multiple statements should be enclosed in a do - while block:
>>   
>>   	#define macrofun(a, b, c)			\
>>   		do {					\
>> -			if (a == 5)			\
>> +			if ((a) == 5)			\
>>   				do_this(b, c);		\
>>   		} while (0)
>>   
>> +Always use parentheses around macro arguments, except for the situations listed
>> +below.
>> +
>> +Examples where parentheses are required around macro arguments:
>> +
>> +.. code-block:: c
>> +
>> +	#define foo(a, b)				\
>> +		do {					\
>> +			(a) = (b);			\
>> +		} while (0)
>> +
>> +.. code-block:: c
>> +
>> +	#define foo(a)					\
>> +		do {					\
>> +			(a)++;				\
>> +		} while (0)
>> +
>> +.. code-block:: c
>> +
>> +	#define cmp_gt(a, b)			((a) > (b))
>> +
>> +.. code-block:: c
>> +
>> +	#define foo(a)				do_this(!(a))
>> +
>> +.. code-block:: c
>> +
>> +	#define foo(a)				do_this(*(a))
>> +
>> +.. code-block:: c
>> +
>> +	#define foo(a)				do_this(&(a))
>> +
>> +.. code-block:: c
>> +
>> +	#define get_member(struct_var)		do_this((struct_var).member)
>> +
>> +.. code-block:: c
>> +
>> +	#define deref_member(struct_ptr)	do_this((struct_ptr)->member)
> 
> I wonder if we really need to give all of these examples?  We've already
> said "always put parentheses except in a few cases" - I would think that
> would be enough.

OK, let's remove those examples for now. If it proves that it causes confusion
we can always add them back.

> 
>> +Situations where parentheses should not be added around arguments, when:
> 
> For these, it would be nice to say *why* parentheses shouldn't be added;
> helping readers understand the reasoning might have more benefit than
> imparting a set of rules.

OK, those would look like:

Always use parentheses around macro arguments, except when:

* they are used as a full expression and negatively impact code readability
   (because the extra parentheses would not have any role in preserving operator
   precedence, making them redundant):

[...]

* they are used as expression within an array subscript operator "[]" (because brackets
   nest just like parentheses themselves do):

[...]

* they are used as arguments to functions or other macros (because the comma
   separator between arguments is not even an operator, so there is no operator
   precedence to preserve):

[...]

(note: the ones below include new additions)

* there is some syntax reason why adding the parentheses would not work, e.g.
   when using the parameter as member name, for string expansion, or token
   pasting:

   * the ``member`` argument:

     .. code-block:: c

         #define foo(struct_p, member)   do_this((struct_p)->member)

   * string expansion:

     .. code-block:: c

         #define __stringify_1(x...)     #x
         #define __stringify(x...)       __stringify_1(x)

   * token pasting:

     .. code-block:: c

         #define COMPAT_SYSCALL_DEFINE1(name, ...)       \
                 COMPAT_SYSCALL_DEFINEx(1, _##name, __VA_ARGS__)

Thanks!

Mathieu


> 
> Thanks,
> 
> jon

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com