bracing the loop in kernel/softirq.c

PATCH In review Cyrill Gorcunov 3 participants 9 messages Jun 20, 2007 – Jun 22, 2007

Share & tools

Cyrill Gorcunov

author

2007-06-20 17:59 UTC #0 lore raw

PATCH 1/1 bracing the loop in kernel/softirq.c

This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.

Signed-off-by Cyrill Gorcunov <[email protected]>

---

 kernel/softirq.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/softirq.c +2 -2

index 0b9886a..d1a7e89 100644
@@ -470,9 +470,9 @@ void tasklet_kill(struct tasklet_struct *t)
 		printk("Attempt to kill tasklet from interrupt\n");
 
 	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
-		do
+		do {
 			yield();
-		while (test_bit(TASKLET_STATE_SCHED, &t->state));
+		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
 	}
 	tasklet_unlock_wait(t);
 	clear_bit(TASKLET_STATE_SCHED, &t->state);

Jesper Juhl

2007-06-20 21:01 UTC #1 lore raw

Quoted 4 lines · Cyrill Gorcunov

This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.

That's generally not done.

It's even in Documentation/CodingStyle :

"
Do not unnecessarily use braces where a single statement will do.

if (condition)

       action();

Signature · 3 lines

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Cyrill Gorcunov

author

2007-06-21 13:52 UTC #2 lore raw

[Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]

| From: Jesper Juhl <[email protected]>
| To: Cyrill Gorcunov <[email protected]>
| Cc: LKML <[email protected]>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Wed, 20 Jun 2007 23:01:44 +0200
|

Quoted 7 lines · Cyrill Gorcunov

Quoted 4 lines
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.

Quoted 14 lines

It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"
Signature · 3 lines
Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Hi Jasper,

look, the CodingStyle is absolutely right BUT:

- dropping the braces are good solution for 'if' statement indeed
- dropping the braces are _not_ good for 'do' - 'while' loop 'case it fails on further loop modifications. Moreover adding these braces we don't change amount of lines in code! So why souldn't we? I don't see any reason not to do.

Cyrill

Jesper Juhl

2007-06-21 17:20 UTC #3 lore raw

Quoted 23 lines · Cyrill Gorcunov

[Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]
| From: Jesper Juhl <[email protected]>
| To: Cyrill Gorcunov <[email protected]>
| Cc: LKML <[email protected]>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Wed, 20 Jun 2007 23:01:44 +0200
|
Quoted 7 lines · Cyrill Gorcunov
Quoted 4 lines
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.
Quoted 8 lines
It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"

Quoted 8 lines

look, the CodingStyle is absolutely right BUT:
- dropping the braces are good solution for 'if' statement indeed
- dropping the braces are _not_ good for 'do' - 'while' loop 'case it fails on further loop modifications. Moreover adding these braces we don't change amount of lines in code! So why souldn't we? I don't see any reason not to do.

Personally, in this case, I don't care. I'm simply telling you that usually that's not a patch that would get accepted, that's all. But it's all up to the maintainer of that area of the kernel (whom you
probably want to at least Cc in addition to just LKML). :-)

Signature · 3 lines

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Cyrill Gorcunov

author

2007-06-21 17:34 UTC #4 lore raw

[Jesper Juhl - Thu, Jun 21, 2007 at 07:20:33PM +0200]

| From: Jesper Juhl <[email protected]>
| To: Cyrill Gorcunov <[email protected]>
| Cc: LKML <[email protected]>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Thu, 21 Jun 2007 19:20:33 +0200
|

Quoted 35 lines · Cyrill Gorcunov

Quoted 25 lines
[Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]
| From: Jesper Juhl <[email protected]>
| To: Cyrill Gorcunov <[email protected]>
| Cc: LKML <[email protected]>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Wed, 20 Jun 2007 23:01:44 +0200
|
Quoted 7 lines · Cyrill Gorcunov
Quoted 4 lines
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.
Quoted 8 lines
It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"
look, the CodingStyle is absolutely right BUT:
Quoted 8 lines
- dropping the braces are good solution for 'if' statement indeed
- dropping the braces are _not_ good for 'do' - 'while' loop 'case it fails on further loop modifications. Moreover adding these
braces
we don't change amount of lines in code! So why souldn't we? I
don't
see any reason not to do.

Quoted 10 lines

Personally, in this case, I don't care. I'm simply telling you that usually that's not a patch that would get accepted, that's all. But it's all up to the maintainer of that area of the kernel (whom you
probably want to at least Cc in addition to just LKML). :-)
Signature · 3 lines
Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Anyway, thanks for comments ;)

Cyrill

Jeremy Fitzhardinge

2007-06-21 18:53 UTC #5 lore raw

In reply to Jesper Juhl

Quoted 7 lines · Jesper Juhl

Quoted 4 lines · Cyrill Gorcunov
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.

Quoted 8 lines

It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"

I tend to see "do {} while()" as an exception to this. I find the construct is sufficiently rare that it helps to emphasize it a bit. For example if I'm scanning code and I see:

	while (foo != NULL);

in the corner of my eye, I'm going to think "huh?". But:

	} while (foo != NULL);

visually "parses" properly, regardless of how near or far the corresponding "do {" is.

(And of course, its consistent with the super extra special while-while loop:

	while (foo != NULL) {
		foo = bar();
		piffle();
	} while (foo != NULL);	// make sure we loop properly

;)

Cyrill Gorcunov

author

2007-06-22 13:57 UTC #6 lore raw

[Jeremy Fitzhardinge - Thu, Jun 21, 2007 at 11:52:25AM -0700]

Quoted 33 lines

Quoted 7 lines · Jesper Juhl
Quoted 4 lines · Cyrill Gorcunov
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.
Quoted 8 lines
It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"
I tend to see "do {} while()" as an exception to this. I find the construct is sufficiently rare that it helps to emphasize it a bit. For example if I'm scanning code and I see:
	while (foo != NULL);
in the corner of my eye, I'm going to think "huh?". But:
	} while (foo != NULL);
visually "parses" properly, regardless of how near or far the corresponding "do {" is.
(And of course, its consistent with the super extra special while-while loop:

Quoted 9 lines

	while (foo != NULL) {
		foo = bar();
		piffle();
	} while (foo != NULL);	// make sure we loop properly

;)

Indeed, 'do-while' is a special case.
Btw, Jeremy, I'm always getting into rapture on 'while-while' loop ;)

Cyrill

Cyrill Gorcunov

author

2007-06-22 15:56 UTC #7 lore raw

In reply to Jeremy Fitzhardinge

[Jeremy Fitzhardinge - Thu, Jun 21, 2007 at 11:52:25AM -0700]

Quoted 33 lines

Quoted 7 lines · Jesper Juhl
Quoted 4 lines · Cyrill Gorcunov
This trivial patch adds braces over a one-line
loop. That makes code...well... little bit
convenient for (possible) further modifications.
That's generally not done.
Quoted 8 lines
It's even in Documentation/CodingStyle :
"
Do not unnecessarily use braces where a single statement will do.
if (condition)
       action();
"
I tend to see "do {} while()" as an exception to this. I find the construct is sufficiently rare that it helps to emphasize it a bit. For example if I'm scanning code and I see:
	while (foo != NULL);
in the corner of my eye, I'm going to think "huh?". But:
	} while (foo != NULL);
visually "parses" properly, regardless of how near or far the corresponding "do {" is.
(And of course, its consistent with the super extra special while-while loop:

Quoted 9 lines

	while (foo != NULL) {
		foo = bar();
		piffle();
	} while (foo != NULL);	// make sure we loop properly

;)

Btw, Jeremy, could you please Ack it then and I'll resend the patch to Andrew ;)

Cyrill

Jeremy Fitzhardinge

2007-06-22 18:06 UTC #8 lore raw

Quoted 2 lines · Cyrill Gorcunov

Btw, Jeremy, could you please Ack it then and I'll resend the patch to Andrew ;)

Not sure its worth a patch to just change that on its own. Fix it in passing if it needs changing anyway, or generate a patch to fix them all. But I can't see Andrew being very keen on churn for the sake of it.