Add an explanation for defining multi-line macros using the ({ })
notation to CodingStyle.
Signed-off-by: Robert P. J. Day <[email protected]>
---
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 9069189..1d0ddb8 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -549,13 +549,24 @@ may be named in lower case.
Generally, inline functions are preferable to macros resembling functions.
-Macros with multiple statements should be enclosed in a do - while block:
+There are two techniques for defining macros that contain multiple
+statements.
-#define macrofun(a, b, c) \
- do { \
+ (a) Enclose those statements in a do - while block:
+
+ #define macrofun(a, b, c) \
+ do { \
+ if (a == 5) \
+ do_this(b, c); \
+ } while (0)
+
+ (b) Use the gcc extension that a compound statement enclosed in
+ parentheses represents an expression:
+
+ #define macrofun(a, b, c) ({ \
if (a == 5) \
do_this(b, c); \
- } while (0)
+ })
Things to avoid when using macros:
On Sun, Dec 31, 2006 at 02:32:25PM -0500, Robert P. J. Day wrote:
> Generally, inline functions are preferable to macros resembling
> functions.
This should be stressed, IMHO. We have too many macros which have no
reason to live.
> -Macros with multiple statements should be enclosed in a do - while block:
> +There are two techniques for defining macros that contain multiple
> +statements.
>
> -#define macrofun(a, b, c) \
> - do { \
> + (a) Enclose those statements in a do - while block:
> +
> + #define macrofun(a, b, c) \
> + do { \
> + if (a == 5) \
> + do_this(b, c); \
> + } while (0)
> +
> + (b) Use the gcc extension that a compound statement enclosed in
> + parentheses represents an expression:
> +
> + #define macrofun(a, b, c) ({ \
> if (a == 5) \
> do_this(b, c); \
> - } while (0)
> + })
When giving two alternatives, the reader will thank you if you explain
when each should be used. In this case, the second form should be used
when the macro needs to return a value (and you can't use an inline
function for whatever reason), whereas the first form should be used
at all other times.
Cheers,
Muli
On Sun, 31 Dec 2006, Muli Ben-Yehuda wrote:
> On Sun, Dec 31, 2006 at 02:32:25PM -0500, Robert P. J. Day wrote:
>
> > Generally, inline functions are preferable to macros resembling
> > functions.
>
> This should be stressed, IMHO. We have too many macros which have no
> reason to live.
>
> > -Macros with multiple statements should be enclosed in a do - while block:
> > +There are two techniques for defining macros that contain multiple
> > +statements.
> >
> > -#define macrofun(a, b, c) \
> > - do { \
> > + (a) Enclose those statements in a do - while block:
> > +
> > + #define macrofun(a, b, c) \
> > + do { \
> > + if (a == 5) \
> > + do_this(b, c); \
> > + } while (0)
> > +
> > + (b) Use the gcc extension that a compound statement enclosed in
> > + parentheses represents an expression:
> > +
> > + #define macrofun(a, b, c) ({ \
> > if (a == 5) \
> > do_this(b, c); \
> > - } while (0)
> > + })
>
> When giving two alternatives, the reader will thank you if you
> explain when each should be used. In this case, the second form
> should be used when the macro needs to return a value (and you can't
> use an inline function for whatever reason), whereas the first form
> should be used at all other times.
that's a fair point, although it's certainly not the coding style
that's in play now. for example,
#define setcc(cc) ({ \
partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
there would appear to be *lots* of cases where the ({ }) notation is
used when nothing is being returned. i'm not sure you can be that
adamant about that distinction at this point.
rday
On Sun, Dec 31, 2006 at 02:49:48PM -0500, Robert P. J. Day wrote:
> there would appear to be *lots* of cases where the ({ }) notation is
> used when nothing is being returned. i'm not sure you can be that
> adamant about that distinction at this point.
IMHO, the main point of CodingStyle is to clarify how new code should
be written and old code should've been written.
Cheers,
Muli
On Sun, 31 Dec 2006 22:09:03 +0200 Muli Ben-Yehuda wrote:
> On Sun, Dec 31, 2006 at 02:49:48PM -0500, Robert P. J. Day wrote:
>
> > there would appear to be *lots* of cases where the ({ }) notation is
> > used when nothing is being returned. i'm not sure you can be that
> > adamant about that distinction at this point.
>
> IMHO, the main point of CodingStyle is to clarify how new code should
> be written and old code should've been written.
I agree. We aren't trying to "fix" ancient history.
---
~Randy
On Sun, 31 Dec 2006, Muli Ben-Yehuda wrote:
> On Sun, Dec 31, 2006 at 02:49:48PM -0500, Robert P. J. Day wrote:
>
> > there would appear to be *lots* of cases where the ({ }) notation
> > is used when nothing is being returned. i'm not sure you can be
> > that adamant about that distinction at this point.
>
> IMHO, the main point of CodingStyle is to clarify how new code
> should be written and old code should've been written.
ok, how about this as a patch:
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 9069189..064a13e 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -549,13 +549,26 @@ may be named in lower case.
Generally, inline functions are preferable to macros resembling functions.
-Macros with multiple statements should be enclosed in a do - while block:
-
-#define macrofun(a, b, c) \
- do { \
- if (a == 5) \
- do_this(b, c); \
- } while (0)
+There are two techniques for defining macros that contain multiple
+statements, depending on whether you're returning a value or not:
+
+ (a) If there is no return value from the macro, you should enclose
+ the statements in a do - while block, as in:
+
+ #define macrofun(a, b, c) \
+ do { \
+ if (a == 5) \
+ do_this(b, c); \
+ } while (0)
+
+ (b) If the macro is designed to return a value, you should use the
+ gcc extension that a compound statement enclosed in parentheses
+ represents an expression, as in:
+
+ #define maxint(a, b) ({ \
+ int _a = (a), _b = (b); \
+ _a > _b ? _a : _b; \
+ })
Things to avoid when using macros:
>> In this case, the second form
>> should be used when the macro needs to return a value (and you can't
>> use an inline function for whatever reason), whereas the first form
>> should be used at all other times.
>
> that's a fair point, although it's certainly not the coding style
> that's in play now. for example,
>
> #define setcc(cc) ({ \
> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
This _does_ return a value though, bad example.
Segher
Segher Boessenkool wrote:
>>> In this case, the second form
>>> should be used when the macro needs to return a value (and you can't
>>> use an inline function for whatever reason), whereas the first form
>>> should be used at all other times.
>>
>> that's a fair point, although it's certainly not the coding style
>> that's in play now. for example,
>>
>> #define setcc(cc) ({ \
>> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
>> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
>
> This _does_ return a value though, bad example.
Where does it return a value? I don't see any uses of it
in arch/i386/math-emu/* that use it as returning a value.
And with a small change to put it inside a do-while block
instead of ({ ... }), it at least builds cleanly.
I expected some complaints.
--
~Randy
>>> #define setcc(cc) ({ \
>>> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
>>> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
>> This _does_ return a value though, bad example.
>
> Where does it return a value?
partial_status |=
> I don't see any uses of it
Ah, that's a separate thing -- it returns a value, it's just
never used.
> And with a small change to put it inside a do-while block
> instead of ({ ... }), it at least builds cleanly.
Well please replace it then, statement expressions should be
avoided where possible (to start with, they don't have well-
defined semantics).
Segher
Segher Boessenkool wrote:
>>>> #define setcc(cc) ({ \
>>>> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
>>>> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
>>> This _does_ return a value though, bad example.
>>
>> Where does it return a value?
>
> partial_status |=
as I expected (or suspected).
I also suspect that it wasn't intended, but this is old code
and I wasn't around Linux when it was written, so I don't know
about it for sure.
>> I don't see any uses of it
>
> Ah, that's a separate thing -- it returns a value, it's just
> never used.
Ack.
>> And with a small change to put it inside a do-while block
>> instead of ({ ... }), it at least builds cleanly.
>
> Well please replace it then, statement expressions should be
> avoided where possible (to start with, they don't have well-
> defined semantics).
We should probably avoid gcc extensions when possible.
I'll send a separate email for the patch.
--
~Randy
On Mon, 1 Jan 2007, Segher Boessenkool wrote:
> > > In this case, the second form
> > > should be used when the macro needs to return a value (and you can't
> > > use an inline function for whatever reason), whereas the first form
> > > should be used at all other times.
> >
> > that's a fair point, although it's certainly not the coding style
> > that's in play now. for example,
> >
> > #define setcc(cc) ({ \
> > partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
> > partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
>
> This _does_ return a value though, bad example.
sigh ... you're right. here's a thought. my original patch
submission simply added an explanation for allowing the ({ }) notation
for defining a multi-line macro, without getting into recommending an
actual coding style. at a minimum, something like that should be
added to the style document.
if someone wants to extend that explanation recommending *when* each
of those two styles should be used, feel free. but a simple
decription of alternatives should *at least* be added, no?
rday
On Sun, Dec 31, 2006 at 02:32:25PM -0500, Robert P. J. Day wrote:
> + (a) Enclose those statements in a do - while block:
> +
> + #define macrofun(a, b, c) \
> + do { \
> + if (a == 5) \
> + do_this(b, c); \
> + } while (0)
nitpick, please don't add an indentaion level for the do {. Do this
should look like:
#define macrofun(a, b, c) \
do { \
if (a == 5) \
do_this(b, c); \
} while (0)
> + (b) Use the gcc extension that a compound statement enclosed in
> + parentheses represents an expression:
> +
> + #define macrofun(a, b, c) ({ \
> if (a == 5) \
> do_this(b, c); \
> - } while (0)
> + })
I'd rather document to not use this - it makes the code far less
redable. And it's a non-standard extension, something we only
use if it provides us a benefit which it doesn't here.
On Mon, 1 Jan 2007, Christoph Hellwig wrote:
> On Sun, Dec 31, 2006 at 02:32:25PM -0500, Robert P. J. Day wrote:
> > + (a) Enclose those statements in a do - while block:
> > +
> > + #define macrofun(a, b, c) \
> > + do { \
> > + if (a == 5) \
> > + do_this(b, c); \
> > + } while (0)
>
> nitpick, please don't add an indentaion level for the do {. Do this
> should look like:
>
> #define macrofun(a, b, c) \
> do { \
> if (a == 5) \
> do_this(b, c); \
> } while (0)
the former is the way it's presented in CodingStyle currently, it
wasn't my personal opinion on the subject. i was just reproducing
what was already there.
> > + (b) Use the gcc extension that a compound statement enclosed in
> > + parentheses represents an expression:
> > +
> > + #define macrofun(a, b, c) ({ \
> > if (a == 5) \
> > do_this(b, c); \
> > - } while (0)
> > + })
>
> I'd rather document to not use this - it makes the code far less
> redable. And it's a non-standard extension, something we only
> use if it provides us a benefit which it doesn't here.
it might be a bit late to put a cork in *that* bottle:
$ grep -r "#define.*({" *
rday
On Dec 31 2006 19:23, Randy Dunlap wrote:
>> >
>> > #define setcc(cc) ({ \
>> > partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
>> > partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); })
>>
>> This _does_ return a value though, bad example.
>
> Where does it return a value? I don't see any uses of it
> in arch/i386/math-emu/* that use it as returning a value.
>
> And with a small change to put it inside a do-while block
> instead of ({ ... }), it at least builds cleanly.
> I expected some complaints.
If people want to return something from a ({ }) construct, they should do it
explicitly, e.g.
#define setcc(cc) ({ \
partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); \
partial_status; \
})
-`J'
--
Robert P. J. Day wrote:
> On Mon, 1 Jan 2007, Christoph Hellwig wrote:
>
>> On Sun, Dec 31, 2006 at 02:32:25PM -0500, Robert P. J. Day wrote:
>>> + (a) Enclose those statements in a do - while block:
>>> +
>>> + #define macrofun(a, b, c) \
>>> + do { \
>>> + if (a == 5) \
>>> + do_this(b, c); \
>>> + } while (0)
>> nitpick, please don't add an indentaion level for the do {. Do this
>> should look like:
>>
>> #define macrofun(a, b, c) \
>> do { \
>> if (a == 5) \
>> do_this(b, c); \
>> } while (0)
>
> the former is the way it's presented in CodingStyle currently, it
> wasn't my personal opinion on the subject. i was just reproducing
> what was already there.
>
>>> + (b) Use the gcc extension that a compound statement enclosed in
>>> + parentheses represents an expression:
>>> +
>>> + #define macrofun(a, b, c) ({ \
>>> if (a == 5) \
>>> do_this(b, c); \
>>> - } while (0)
>>> + })
>> I'd rather document to not use this - it makes the code far less
>> redable. And it's a non-standard extension, something we only
>> use if it provides us a benefit which it doesn't here.
>
> it might be a bit late to put a cork in *that* bottle:
>
> $ grep -r "#define.*({" *
We aren't trying to prevent its past use. We just aren't encouraging
the use of gcc extensions if there are reasonable & better ways to
do something.
--
~Randy
> If people want to return something from a ({ }) construct, they should
> do it
> explicitly, e.g.
>
> #define setcc(cc) ({ \
> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); \
> partial_status; \
> })
No, they generally should use an inline function instead.
Segher
On Jan 1 2007 18:51, Segher Boessenkool wrote:
>> If people want to return something from a ({ }) construct, they should do
>> it
>> explicitly, e.g.
>>
>> #define setcc(cc) ({ \
>> partial_status &= ~(SW_C0|SW_C1|SW_C2|SW_C3); \
>> partial_status |= (cc) & (SW_C0|SW_C1|SW_C2|SW_C3); \
>> partial_status; \
>> })
>
> No, they generally should use an inline function instead.
Perhaps. But that won't work with defines where typeof is involved.
-`J'
--