| Message ID | 20230504200527.1935944-13-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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(-)