I tried to look at various list and list user headers in the kernel tree
to figure out what the proper parentheses pattern should be around macro
parameter use for new code, and it turns out that the current code base
lacks consistency.
This is not an exhaustive change of all public kernel headers, but at
least it is a start, updating those which are implementing or using
kernel lists.
The basic rules followed here are:
- Use parentheses around arguments which are rvalues, except when those
are expressions between commas "," used as arguments to functions or
other macros, or surrounded by brackets "[]".
- Do not use parentheses around arguments which are lvalues.
For consistency, when a macro argument is used both as an lvalue and as
an rvalue within the macro, use the parentheses rules applying to each
of the context: with parentheses for rvalue, without parentheses for
lvalue.
Mathieu Desnoyers (13):
rcu: rcupdate.h: Fix parentheses around macro parameter use
rculist.h: Fix parentheses around macro pointer parameter use
rculist_nulls.h: Add parentheses around macro pointer parameter use
rculist_bl.h: Fix parentheses around macro pointer parameter use
list.h: Fix parentheses around macro pointer parameter use
list_nulls.h: Fix parentheses around macro pointer parameter use
list_bl.h: Fix parentheses around macro pointer parameter use
llist.h: Fix parentheses around macro pointer parameter use
klist.h: Fix parentheses around macro parameter use
resource_ext.h: Remove useless parentheses around macro parameters
netdevice.h: Fix parentheses around macro parameter use
blk-mq.h: Fix parentheses around macro parameter use
bio.h: Fix parentheses around macro parameter use
include/linux/bio.h | 22 +++++++-------
include/linux/blk-mq.h | 38 ++++++++++++------------
include/linux/klist.h | 8 +++---
include/linux/list.h | 54 +++++++++++++++++------------------
include/linux/list_bl.h | 12 ++++----
include/linux/list_nulls.h | 8 +++---
include/linux/llist.h | 14 ++++-----
include/linux/netdevice.h | 12 ++++----
include/linux/rculist.h | 28 +++++++++---------
include/linux/rculist_bl.h | 6 ++--
include/linux/rculist_nulls.h | 4 +--
include/linux/rcupdate.h | 46 ++++++++++++++---------------
include/linux/resource_ext.h | 6 ++--
13 files changed, 129 insertions(+), 129 deletions(-)
--
2.25.1
Add missing parentheses around macro parameter use in the following
pattern:
- "x - 1" changed for "(x) - 1",
- "x->member" changed for "(x)->member".
to ensure operator precedence behaves as expected.
Remove useless parentheses around macro parameter use in the following
pattern:
- m((x), y) changed for m(x, y), because comma has the lowest operator
precedence, which makes the extra comma useless.
Signed-off-by: Mathieu Desnoyers <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
---
include/linux/netdevice.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 08fbd4622ccf..5a357262a45b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -283,7 +283,7 @@ struct hh_cache {
/* cached hardware header; allow for machine alignment needs. */
#define HH_DATA_MOD 16
#define HH_DATA_OFF(__len) \
- (HH_DATA_MOD - (((__len - 1) & (HH_DATA_MOD - 1)) + 1))
+ (HH_DATA_MOD - ((((__len) - 1) & (HH_DATA_MOD - 1)) + 1))
#define HH_DATA_ALIGN(__len) \
(((__len)+(HH_DATA_MOD-1))&~(HH_DATA_MOD - 1))
unsigned long hh_data[HH_DATA_ALIGN(LL_MAX_HEADER) / sizeof(long)];
@@ -4459,7 +4459,7 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
}
#define HARD_TX_LOCK(dev, txq, cpu) { \
- if ((dev->features & NETIF_F_LLTX) == 0) { \
+ if (((dev)->features & NETIF_F_LLTX) == 0) { \
__netif_tx_lock(txq, cpu); \
} else { \
__netif_tx_acquire(txq); \
@@ -4467,12 +4467,12 @@ static inline void netif_tx_unlock_bh(struct net_device *dev)
}
#define HARD_TX_TRYLOCK(dev, txq) \
- (((dev->features & NETIF_F_LLTX) == 0) ? \
+ ((((dev)->features & NETIF_F_LLTX) == 0) ? \
__netif_tx_trylock(txq) : \
__netif_tx_acquire(txq))
#define HARD_TX_UNLOCK(dev, txq) { \
- if ((dev->features & NETIF_F_LLTX) == 0) { \
+ if (((dev)->features & NETIF_F_LLTX) == 0) { \
__netif_tx_unlock(txq); \
} else { \
__netif_tx_release(txq); \
@@ -4534,7 +4534,7 @@ static inline void netif_addr_unlock_bh(struct net_device *dev)
* rcu_read_lock held.
*/
#define for_each_dev_addr(dev, ha) \
- list_for_each_entry_rcu(ha, &dev->dev_addrs.list, list)
+ list_for_each_entry_rcu(ha, &(dev)->dev_addrs.list, list)
/* These functions live elsewhere (drivers/net/net_init.c, but related) */
@@ -5237,6 +5237,6 @@ extern struct net_device *blackhole_netdev;
/* Note: Avoid these macros in fast path, prefer per-cpu or per-queue counters. */
#define DEV_STATS_INC(DEV, FIELD) atomic_long_inc(&(DEV)->stats.__##FIELD)
#define DEV_STATS_ADD(DEV, FIELD, VAL) \
- atomic_long_add((VAL), &(DEV)->stats.__##FIELD)
+ atomic_long_add(VAL, &(DEV)->stats.__##FIELD)
#endif /* _LINUX_NETDEVICE_H */
--
2.25.1
Add missing parentheses around "_name" parameter use in KLIST_INIT().
It keeps list macros consistent, and prevents unexpected operator
precedence situations, for example:
struct klist a[1] = { KLIST_INIT(*a, NULL, NULL) };
Where the "." operator within KLIST_INIT() is evaluated before the "*"
operator.
Add missing parentheses around macro parameter use in the following
patterns to ensure operator precedence behaves as expected:
- "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.
Signed-off-by: Mathieu Desnoyers <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
include/linux/klist.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/klist.h b/include/linux/klist.h
index b0f238f20dbb..d7e0612ca4ff 100644
--- a/include/linux/klist.h
+++ b/include/linux/klist.h
@@ -23,10 +23,10 @@ struct klist {
} __attribute__ ((aligned (sizeof(void *))));
#define KLIST_INIT(_name, _get, _put) \
- { .k_lock = __SPIN_LOCK_UNLOCKED(_name.k_lock), \
- .k_list = LIST_HEAD_INIT(_name.k_list), \
- .get = _get, \
- .put = _put, }
+ { .k_lock = __SPIN_LOCK_UNLOCKED((_name).k_lock), \
+ .k_list = LIST_HEAD_INIT((_name).k_list), \
+ .get = (_get), \
+ .put = (_put), }
#define DEFINE_KLIST(_name, _get, _put) \
struct klist _name = KLIST_INIT(_name, _get, _put)
--
2.25.1
Parentheses around macro parameters which are surrounded by commas is
a scenario where those added parentheses are useless, because the comma
is the operator with the lowest precedence.
Remove those useless parentheses to make list iteration code consistent
across kernel headers.
Signed-off-by: Mathieu Desnoyers <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Jiang Liu <[email protected]>
---
include/linux/resource_ext.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
index ff0339df56af..f4a3c0040886 100644
--- a/include/linux/resource_ext.h
+++ b/include/linux/resource_ext.h
@@ -60,11 +60,11 @@ resource_list_destroy_entry(struct resource_entry *entry)
resource_list_free_entry(entry);
}
-#define resource_list_for_each_entry(entry, list) \
- list_for_each_entry((entry), (list), node)
+#define resource_list_for_each_entry(entry, list) \
+ list_for_each_entry(entry, list, node)
#define resource_list_for_each_entry_safe(entry, tmp, list) \
- list_for_each_entry_safe((entry), (tmp), (list), node)
+ list_for_each_entry_safe(entry, tmp, list, node)
static inline struct resource_entry *
resource_list_first_type(struct list_head *list, unsigned long type)
--
2.25.1
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: [email protected]
---
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)
#define rq_list_add_tail(lastpptr, rq) do { \
(rq)->rq_next = NULL; \
- **(lastpptr) = rq; \
- *(lastpptr) = &rq->rq_next; \
+ **(lastpptr) = (rq); \
+ *(lastpptr) = &(rq)->rq_next; \
} while (0)
#define rq_list_pop(listptr) \
@@ -251,11 +251,11 @@ static inline unsigned short req_get_ioprio(struct request *req)
})
#define rq_list_for_each(listptr, pos) \
- for (pos = rq_list_peek((listptr)); pos; pos = rq_list_next(pos))
+ for (pos = rq_list_peek(listptr); pos; pos = rq_list_next(pos))
#define rq_list_for_each_safe(listptr, pos, nxt) \
- for (pos = rq_list_peek((listptr)), nxt = rq_list_next(pos); \
- pos; pos = nxt, nxt = pos ? rq_list_next(pos) : NULL)
+ for (pos = rq_list_peek(listptr), nxt = rq_list_next(pos); \
+ pos; pos = (nxt), nxt = (pos) ? rq_list_next(pos) : NULL)
#define rq_list_next(rq) (rq)->rq_next
#define rq_list_empty(list) ((list) == (struct request *) NULL)
@@ -692,10 +692,10 @@ enum {
BLK_MQ_CPU_WORK_BATCH = 8,
};
#define BLK_MQ_FLAG_TO_ALLOC_POLICY(flags) \
- ((flags >> BLK_MQ_F_ALLOC_POLICY_START_BIT) & \
+ (((flags) >> BLK_MQ_F_ALLOC_POLICY_START_BIT) & \
((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1))
#define BLK_ALLOC_POLICY_TO_MQ_FLAG(policy) \
- ((policy & ((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1)) \
+ (((policy) & ((1 << BLK_MQ_F_ALLOC_POLICY_BITS) - 1)) \
<< BLK_MQ_F_ALLOC_POLICY_START_BIT)
#define BLK_MQ_NO_HCTX_IDX (-1U)
@@ -948,11 +948,11 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
}
#define queue_for_each_hw_ctx(q, hctx, i) \
- xa_for_each(&(q)->hctx_table, (i), (hctx))
+ xa_for_each(&(q)->hctx_table, i, hctx)
#define hctx_for_each_ctx(hctx, ctx, i) \
- for ((i) = 0; (i) < (hctx)->nr_ctx && \
- ({ ctx = (hctx)->ctxs[(i)]; 1; }); (i)++)
+ for (i = 0; (i) < (hctx)->nr_ctx && \
+ ({ ctx = (hctx)->ctxs[i]; 1; }); (i)++)
static inline void blk_mq_cleanup_rq(struct request *rq)
{
@@ -1013,20 +1013,20 @@ struct req_iterator {
};
#define __rq_for_each_bio(_bio, rq) \
- if ((rq->bio)) \
- for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next)
+ if ((rq)->bio) \
+ for (_bio = (rq)->bio; _bio; _bio = (_bio)->bi_next)
#define rq_for_each_segment(bvl, _rq, _iter) \
- __rq_for_each_bio(_iter.bio, _rq) \
- bio_for_each_segment(bvl, _iter.bio, _iter.iter)
+ __rq_for_each_bio((_iter).bio, _rq) \
+ bio_for_each_segment(bvl, (_iter).bio, (_iter).iter)
#define rq_for_each_bvec(bvl, _rq, _iter) \
- __rq_for_each_bio(_iter.bio, _rq) \
- bio_for_each_bvec(bvl, _iter.bio, _iter.iter)
+ __rq_for_each_bio((_iter).bio, _rq) \
+ bio_for_each_bvec(bvl, (_iter).bio, (_iter).iter)
#define rq_iter_last(bvec, _iter) \
- (_iter.bio->bi_next == NULL && \
- bio_iter_last(bvec, _iter.iter))
+ ((_iter).bio->bi_next == NULL && \
+ bio_iter_last(bvec, (_iter).iter))
/*
* blk_rq_pos() : the current sector
--
2.25.1
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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Fri, May 5, 2023 at 6:56 AM Mathieu Desnoyers
<[email protected]> 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
> <[email protected]> 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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Thu, 4 May 2023 16:05:25 -0400 Mathieu Desnoyers wrote:
> Add missing parentheses around macro parameter use in the following
> pattern:
>
> - "x - 1" changed for "(x) - 1",
> - "x->member" changed for "(x)->member".
>
> to ensure operator precedence behaves as expected.
>
> Remove useless parentheses around macro parameter use in the following
> pattern:
>
> - m((x), y) changed for m(x, y), because comma has the lowest operator
> precedence, which makes the extra comma useless.
Sure, why not. Can we take it via netdev, tho?
I can't have any dependencies, right?
On 2023-05-05 14:44, Jakub Kicinski wrote:
> On Thu, 4 May 2023 16:05:25 -0400 Mathieu Desnoyers wrote:
>> Add missing parentheses around macro parameter use in the following
>> pattern:
>>
>> - "x - 1" changed for "(x) - 1",
>> - "x->member" changed for "(x)->member".
>>
>> to ensure operator precedence behaves as expected.
>>
>> Remove useless parentheses around macro parameter use in the following
>> pattern:
>>
>> - m((x), y) changed for m(x, y), because comma has the lowest operator
>> precedence, which makes the extra comma useless.
>
> Sure, why not. Can we take it via netdev, tho?
> I can't have any dependencies, right?
I'll note your Acked-by in my patch for the next round. When I post
without RFC tag it will be ready for merging. There is still some
high-level feedback I want to gather on the overall series before I
remove the RFC tag.
There are no dependencies, each patch in this series only target a
single header, and there are no dependencies across those patches.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On 2023-05-05 15:54, Linus Torvalds wrote:
> On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
> <[email protected]> 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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Fri, May 5, 2023 at 11:49 AM Mathieu Desnoyers
<[email protected]> 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 Fri, May 5, 2023 at 1:08 PM Mathieu Desnoyers
<[email protected]> 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
> <[email protected]> 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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com