[PATCH] bracing the loop in kernel/softirq.c

[PATCH] bracing the loop in kernel/softirq.c

[Cyrill Gorcunov]

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 <gorcunov@gmail.com>
---

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


diff --git a/kernel/softirq.c b/kernel/softirq.c
index 0b9886a..d1a7e89 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -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);

Re: [PATCH] bracing the loop in kernel/softirq.c

[Jesper Juhl]

On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 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();
"

-- 
Jesper Juhl <jesper.juhl@gmail.com>
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

Re: [PATCH] bracing the loop in kernel/softirq.c

[Cyrill Gorcunov]

[Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]
| From: Jesper Juhl <jesper.juhl@gmail.com>
| To: Cyrill Gorcunov <gorcunov@gmail.com>
| Cc: LKML <linux-kernel@vger.kernel.org>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Wed, 20 Jun 2007 23:01:44 +0200
| 
> On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> 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();
> "
>
> -- 
> Jesper Juhl <jesper.juhl@gmail.com>
> 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

Re: [PATCH] bracing the loop in kernel/softirq.c

[Jesper Juhl]

On 21/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]
> | From: Jesper Juhl <jesper.juhl@gmail.com>
> | To: Cyrill Gorcunov <gorcunov@gmail.com>
> | Cc: LKML <linux-kernel@vger.kernel.org>
> | Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
> | Date: Wed, 20 Jun 2007 23:01:44 +0200
> |
> > On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> >> 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();
> > "
>
> 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).   :-)

-- 
Jesper Juhl <jesper.juhl@gmail.com>
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

Re: [PATCH] bracing the loop in kernel/softirq.c

[Cyrill Gorcunov]

[Jesper Juhl - Thu, Jun 21, 2007 at 07:20:33PM +0200]
| From: Jesper Juhl <jesper.juhl@gmail.com>
| To: Cyrill Gorcunov <gorcunov@gmail.com>
| Cc: LKML <linux-kernel@vger.kernel.org>
| Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
| Date: Thu, 21 Jun 2007 19:20:33 +0200
| 
> On 21/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> [Jesper Juhl - Wed, Jun 20, 2007 at 11:01:44PM +0200]
>> | From: Jesper Juhl <jesper.juhl@gmail.com>
>> | To: Cyrill Gorcunov <gorcunov@gmail.com>
>> | Cc: LKML <linux-kernel@vger.kernel.org>
>> | Subject: Re: [PATCH] bracing the loop in kernel/softirq.c
>> | Date: Wed, 20 Jun 2007 23:01:44 +0200
>> |
>> > On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> >> 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();
>> > "
>>
>> 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).   :-)
>
> -- 
> Jesper Juhl <jesper.juhl@gmail.com>
> 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

Re: [PATCH] bracing the loop in kernel/softirq.c

[Jeremy Fitzhardinge]

Jesper Juhl wrote:
> On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> 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();
> "

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

;)

    J

Re: [PATCH] bracing the loop in kernel/softirq.c

[Cyrill Gorcunov]

[Jeremy Fitzhardinge - Thu, Jun 21, 2007 at 11:52:25AM -0700]
> Jesper Juhl wrote:
>> On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>> 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();
>> "
>
> 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
>
> ;)
>
>    J
>

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

		Cyrill

Re: [PATCH] bracing the loop in kernel/softirq.c

[Cyrill Gorcunov]

[Jeremy Fitzhardinge - Thu, Jun 21, 2007 at 11:52:25AM -0700]
> Jesper Juhl wrote:
>> On 20/06/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>> 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();
>> "
>
> 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
>
> ;)
>
>    J
>

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

		Cyrill

Re: [PATCH] bracing the loop in kernel/softirq.c

[Jeremy Fitzhardinge]

Cyrill Gorcunov wrote:
> 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.

    J