[RFC,12/13] blk-mq.h: Fix parentheses around macro parameter use

  Fix the following macro parameter usage patterns in blk-mq.h for
consistency, ensuring that operator precedence is respected:

Added parentheses:

- x->member is changed for (x)->member,
- x.member is changed for (x).member,
- flags >> BLK_MQ_F_ALLOC_POLICY_START_BIT is changed for
  (flags) >> BLK_MQ_F_ALLOC_POLICY_START_BIT.
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency.

Removed parentheses:

- m((x)) is changed for m(x) (the extra parentheses are useless),
- m(x, (y), (z)) is changed for m(x, y, z), because comma is the lowest
  priority operator, and thus the extra parentheses are useless,
- v[(x)] is changed for v[x], because the extra parentheses are useless
  given that [] already surrounds an expression,
- "(i) = 0" is changed for "i = 0", because "i" is an lvalue, which
  makes the extra parentheses useless.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
---
 include/linux/blk-mq.h | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
  

On 2023-05-04 16:05, Mathieu Desnoyers wrote:
> Fix the following macro parameter usage patterns in blk-mq.h for
> consistency, ensuring that operator precedence is respected:
> 
> Added parentheses:
[...]
> - "x = y" is changed for "x = (y)", because "y" can be an expression
>    containing a comma if it is the result of the expansion of a macro such
>    as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>    precedence. This use-case is far-fetched, but we have to choose one
>    way or the other (with or without parentheses) for consistency.

[...]

>   include/linux/blk-mq.h | 38 +++++++++++++++++++-------------------
>   1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 06caacd77ed6..4de6ad92530c 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -223,13 +223,13 @@ static inline unsigned short req_get_ioprio(struct request *req)
>   
>   #define rq_list_add(listptr, rq)	do {		\
>   	(rq)->rq_next = *(listptr);			\
> -	*(listptr) = rq;				\
> +	*(listptr) = (rq);				\
>   } while (0)
> 

Linus,

Which way do we want to go with respect to the rvalue of the assignment 
operator "=" in a macro ? (with or without parentheses)

In short:

#define m(x) do { z = (x); } while (0)

or

#define m(x) do { z = x; } while (0)

?

Given that "=" has the lowest operator precedence just above comma, and 
its associativity is right-to-left, I suspect the only use that would 
break it without the extra parentheses around "x" is:

#define eval(...) __VA_ARGS__
#define m(x) do { z = x; } while (0)

m(eval(1, abc))

Which generates the following C code after preprocessing:

do { z = 1, abc; } while (0)

which ends up expanding the comma within the rvalue. But this use-case 
is a bit far-fetched, so I don't know if we want to require the 
parentheses or not.

And if we decide that we do want to require the parentheses around the 
"x" parameter in the "=" rvalue, then this means we have to consider 
whether we want to require parentheses around the macro arguments used 
as function/macro arguments, e.g.:

#define eval(...) __VA_ARGS__
#define m(x)    f(x)

m(eval(1, abc));

Which generates the following C code after preprocessing:

f(1, abc);

If we want to be consistent, I suspect we want to require the same for 
both use-cases ("=" rvalue and function/macro parameters).

Thanks,

Mathieu
  

On Fri, May 5, 2023 at 6:56 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Which way do we want to go with respect to the rvalue of the assignment
> operator "=" in a macro ? (with or without parentheses)
>
> In short:
>
> #define m(x) do { z = (x); } while (0)
>
> or
>
> #define m(x) do { z = x; } while (0)

I suspect that the first one is preferred, just as a "don't even have
to think about it" thing.

In general, despite my suggestion of maybe using upper-case to show
odd syntax (and I may have suggested it, but I really don't like how
it looks, so I'm not at all convinced it's a good idea), to a
first-order approximation the rule should be:

 - always use parentheses around macros

 - EXCEPT:
     - when used purely as arguments to functions or other macros
     - when there is some syntax reason why it's not ok to add parens

The "arguments to functions/macros" is because the comma separator
between arguments isn't even a operator (ie it is *not* a
comma-expression, it's multiple expressions separated by commas).
There is no "operator precedence" subtlety.

So we have a lot of macros that are just wrappers around functions (or
other macros), and in that situation you do *not* then add more
parentheses, and doing something like

    #define update_screen(x) redraw_screen(x, 0)

is fine, and might even be preferred syntax because putting
parentheses around 'x' not only doesn't buy you anything, but just
makes things uglier.

And the "syntax reasons" can be due to the usual things: we not only
have that 'pass member name around' issue, but we have things like
string expansion etc, where adding parentheses anywhere to things like

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

would obviously simply not work (or look at our "SYSCALL_DEFINEx()"
games for more complex examples with many layers of token pasting
etc).

But in general I would suggest against "this is the lowest priority
operator" kind of games. Nobody remembers the exact operator
precedence so well that they don't have to think about it.

So for example, we have

    #define scr_writew(val, addr) (*(addr) = (val))

to pick another VT example, and I think that's right both for 'addr'
(that requires the parentheses) and for 'val' (that might not require
it, but let's not make people think about it).

                  Linus
  

On 2023-05-05 14:40, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 6:56 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Which way do we want to go with respect to the rvalue of the assignment
>> operator "=" in a macro ? (with or without parentheses)
>>
>> In short:
>>
>> #define m(x) do { z = (x); } while (0)
>>
>> or
>>
>> #define m(x) do { z = x; } while (0)
> 
> I suspect that the first one is preferred, just as a "don't even have
> to think about it" thing.
> 
> In general, despite my suggestion of maybe using upper-case to show
> odd syntax (and I may have suggested it, but I really don't like how
> it looks, so I'm not at all convinced it's a good idea), to a
> first-order approximation the rule should be:
> 
>   - always use parentheses around macros
> 
>   - EXCEPT:
>       - when used purely as arguments to functions or other macros
>       - when there is some syntax reason why it's not ok to add parens

I would add to this list of exceptions cases where the argument is used 
as an expression within brackets, e.g.

#define m(x) myvar[x]

Because the content within the brackets is already an expression.

The other exception I would add is when a parameter is used as an 
lvalue, as:

#define m(x) do { x = 2; } while (0)

because I cannot find a case where it would cause unexpected operator 
precedence.

Are you OK with those 2 additional exceptions ?

> 
> The "arguments to functions/macros" is because the comma separator
> between arguments isn't even a operator (ie it is *not* a
> comma-expression, it's multiple expressions separated by commas).
> There is no "operator precedence" subtlety.

Good point.

> 
> So we have a lot of macros that are just wrappers around functions (or
> other macros), and in that situation you do *not* then add more
> parentheses, and doing something like
> 
>      #define update_screen(x) redraw_screen(x, 0)
> 
> is fine, and might even be preferred syntax because putting
> parentheses around 'x' not only doesn't buy you anything, but just
> makes things uglier.
> 
> And the "syntax reasons" can be due to the usual things: we not only
> have that 'pass member name around' issue, but we have things like
> string expansion etc, where adding parentheses anywhere to things like
> 
>      #define __stringify_1(x...)     #x
>      #define __stringify(x...)       __stringify_1(x)
> 
> would obviously simply not work (or look at our "SYSCALL_DEFINEx()"
> games for more complex examples with many layers of token pasting
> etc).
> 
> But in general I would suggest against "this is the lowest priority
> operator" kind of games. Nobody remembers the exact operator
> precedence so well that they don't have to think about it.
> 
> So for example, we have
> 
>      #define scr_writew(val, addr) (*(addr) = (val))
> 
> to pick another VT example, and I think that's right both for 'addr'
> (that requires the parentheses) and for 'val' (that might not require
> it, but let's not make people think about it).

Indeed, brain power and reviewer time is a scarce resource. It's a shame 
to waste it on figuring out operator priority again and again.

Thanks,

Mathieu
  

On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> I would add to this list of exceptions cases where the argument is used
> as an expression within brackets, e.g.
>
> #define m(x) myvar[x]

Yeah, that makes sense not because of any operator precedence rules,
but simply because brackets end up syntactically nesting just like
parentheses themselves do.

IOW, while you can mess up that nesting by having non-nested brackets
in the argument, that's equally true of any added parentheses too.

> The other exception I would add is when a parameter is used as an
> lvalue, as:
>
> #define m(x) do { x = 2; } while (0)

I really don't understand why you think '=' is so special. It's very
much not special.

It happens to have the lowest precedence, sure, but the keyword is "happens".

I think you are confused by the non-C languages that make assignment
be not an expression operator, but a statement.

So I think you are technically correct in that the parentheses aren't
_needed_, but the above is still the same case that in many other
situations parentheses aren't technically *needed*, but not having to
think about it is better than having to do so.

           Linus
  

On 2023-05-05 15:54, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:

[...]

>> The other exception I would add is when a parameter is used as an
>> lvalue, as:
>>
>> #define m(x) do { x = 2; } while (0)
> 
> I really don't understand why you think '=' is so special. It's very
> much not special.
> 
> It happens to have the lowest precedence, sure, but the keyword is "happens".
> 
> I think you are confused by the non-C languages that make assignment
> be not an expression operator, but a statement.

The reason why I think the lvalue of a "=" operator can be argued to be 
"special" is because it is simply invalid to apply many of the C 
operators to an lvalue (e.g. +, -, /, ...), which leads me to think that 
there are no valid lvalue parameters which can cause unexpected operator 
precedence.

That being said, just having to *think* about it is wasted brain power, 
so I am in favor of just adding the parentheses for lvalues as well.

> So I think you are technically correct in that the parentheses aren't
> _needed_, but the above is still the same case that in many other
> situations parentheses aren't technically *needed*, but not having to
> think about it is better than having to do so.

Yes, so no exception for the lvalue of an assignment, therefore giving:

#define m(x) do { (x) = 2; } while (0)

If we are OK with this, I will go ahead and update my patch set accordingly.

Thanks,

Mathieu
  

On Fri, May 5, 2023 at 1:08 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> The reason why I think the lvalue of a "=" operator can be argued to be
> "special" is because it is simply invalid to apply many of the C
> operators to an lvalue (e.g. +, -, /, ...),

Mathieu, you are simply objectively wrong.

See here:

  #define m1(x) (x = 2)
  #define m2(x) ((x) = 2)

and then try using the argument "a = b" to those macros.

Guess which one flags it as an error ("lvalue required") and which one does not?

m2 is the only "good" one. Yes, m1 works in 99% of all cases in
practice, but if you want a safer macro, you *will* add the
parentheses.

So *STOP*ARGUING* based on an incorrect "lowest precedence" basis.
Even for the "lowest precedence" case, you have the *same* precedence.

The fact is, assignment is not in any way special operation in macros,
and does not deserve - and should absolutely not have - any special
"doesn't need parentheses around argument" rules.

           Linus
  

On 2023-05-05 16:22, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 1:08 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> The reason why I think the lvalue of a "=" operator can be argued to be
>> "special" is because it is simply invalid to apply many of the C
>> operators to an lvalue (e.g. +, -, /, ...),
> 
> Mathieu, you are simply objectively wrong.
> 
> See here:
> 
>    #define m1(x) (x = 2)
>    #define m2(x) ((x) = 2)
> 
> and then try using the argument "a = b" to those macros.
> 
> Guess which one flags it as an error ("lvalue required") and which one does not?

I'm glad you are proving me wrong. So it was just a lack of imagination 
on my end.

> 
> m2 is the only "good" one. Yes, m1 works in 99% of all cases in
> practice, but if you want a safer macro, you *will* add the
> parentheses.
> 
> So *STOP*ARGUING* based on an incorrect "lowest precedence" basis.
> Even for the "lowest precedence" case, you have the *same* precedence.

Yes, your example clearly shows it.

> The fact is, assignment is not in any way special operation in macros,
> and does not deserve - and should absolutely not have - any special
> "doesn't need parentheses around argument" rules.

Good point. You are right. So that strongly supports the parentheses 
around use of parameters as lvalues. One less special-case to care 
about, which is great.

Thanks,

Mathieu
  

From: Linus Torvalds
> Sent: 05 May 2023 20:55
....
> > The other exception I would add is when a parameter is used as an
> > lvalue, as:
> >
> > #define m(x) do { x = 2; } while (0)
> 
> I really don't understand why you think '=' is so special. It's very
> much not special.
> 
> It happens to have the lowest precedence, sure, but the keyword is "happens".

And consider what happens if you try:
	m(a ? b : c)

Personally I'd avoid using parameters as lvalues if at all possible.
It is much better to have:
	#define m(x) do { *(x) = 2; } while (0)
and require the caller do m(&foo) to make it obvious the value is changed.
(Apart from loop definitions...)

Things like the C++ 'int &arg' make it hard to scan read/search
code for places where variables get changed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

I've attempted to capture the resulting rules based on our discussion to add this to
coding-style.rst. Please let me know if anything is wrong:

(to be added in section 12) Macros, Enums and RTL)

Always use parentheses around macro arguments, except when:

- they are used as a full expression:
   - as an initializer,
   - as an expression statement,
   - as the controlling expression of a selection statement (``if`` or ``switch``),
   - as the controlling expression of a ``while`` or ``do`` statement,
   - as any of the expressions of a ``for`` statement,
   - as the expression in a return statement,
- they are used as expression within an array subscript operator "[]",
- they are used as arguments to functions or other macros,
- there is some syntax reason why adding the parentheses would not work.

(note: I'm unsure about requiring or not the parentheses around initializers.
Based on C11 section "6.8 Statement and blocks", initializers that are not part of
a compound literal are full expressions, which makes the extra parentheses useless.)

Thanks,

Mathieu