[PATCH] teach checkpatch.pl about list_for_each

[PATCH] teach checkpatch.pl about list_for_each

[Christer Weinigel]

Hi Andy,

you seem to be the last person messing around with checkpatch.pl so I'm
addressing this to you. :-)

checkpatch complains about the following:

WARNING: no space between function name and open parenthesis '('
#520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
+       list_for_each_entry (transfer, &message->transfers, transfer_list) {

which I think is a bit bogus since it actually is a for statement in
disguise.  The following patch adds list_for_each to the list of things
that look like functions that it shouldn't complain about.

By the way, what is the consensus on lines over 80 characters?
checkpatch complains about the following:

WARNING: line over 80 characters
#762: FILE: drivers/spi/spi_s3c24xx_dma.c:720:
+       printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav Technologies AB\n");

I can of course break this into:

        printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav "
	       "Technologies AB\n");

but in my opinion that becomes more even unreadable.  Would it be
possible to add a special case so that checkpatch ignores long strings
that go beyond 80 characters?  Do you think it is a good idea?

  /Christer

Index: linux-2.6.23/scripts/checkpatch.pl
===================================================================
--- linux-2.6.23.orig/scripts/checkpatch.pl
+++ linux-2.6.23/scripts/checkpatch.pl
@@ -681,7 +681,7 @@ sub process {
 
 # check for spaces between functions and their parentheses.
 		if ($line =~ /($Ident)\s+\(/ &&
-		    $1 !~ /^(?:if|for|while|switch|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ &&
+		    $1 !~ /^(?:if|for|while|switch|list_for_each.*|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/ &&
 		    $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
 			WARN("no space between function name and open parenthesis '('\n" . $herecurr);
 		}

Re: [PATCH] teach checkpatch.pl about list_for_each

[Arnaldo Carvalho de Melo]

Em Sun, Dec 02, 2007 at 01:03:35PM +0100, Christer Weinigel escreveu:
> Hi Andy,
> 
> you seem to be the last person messing around with checkpatch.pl so I'm
> addressing this to you. :-)
> 
> checkpatch complains about the following:
> 
> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> +       list_for_each_entry (transfer, &message->transfers, transfer_list) {
> 
> which I think is a bit bogus since it actually is a for statement in
> disguise.  The following patch adds list_for_each to the list of things
> that look like functions that it shouldn't complain about.

Then you would have to do this for tons other *_for_each*, such as
hlist_for_each, etc, but:

[acme@doppio net-2.6.25]$ find . -name "*.[ch]" | xargs grep
'_for_each[a-z_]*(' | wc -l
4370
[acme@doppio net-2.6.25]$ find . -name "*.[ch]" | xargs grep
'_for_each[a-z_]* (' | wc -l
160
[acme@doppio net-2.6.25]$

I'd say that the common practice in the * _for_each_* use is to do just
what checkpatch does right now, complain if the is a space. Ah, and that
is also my personal preference 8-)

- Arnaldo

Re: [PATCH] teach checkpatch.pl about list_for_each

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Sun, 02 Dec 2007 13:03:35 +0100, Christer Weinigel said:

> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> +       list_for_each_entry (transfer, &message->transfers, transfer_list) {
> 
> which I think is a bit bogus since it actually is a for statement in
> disguise.

That's how it's *implemented*.  I obviously studied too much Lisp as an
undergrad, I keep thinking of list_for_* helpers as functions that take a
lambda-expression as input. In which case, it's a function and no blank is used.

:)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] teach checkpatch.pl about list_for_each

[Andy Whitcroft]

On Sun, Dec 02, 2007 at 01:03:35PM +0100, Christer Weinigel wrote:
> Hi Andy,
> 
> you seem to be the last person messing around with checkpatch.pl so I'm
> addressing this to you. :-)
> 
> checkpatch complains about the following:
> 
> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> +       list_for_each_entry (transfer, &message->transfers, transfer_list) {
> 
> which I think is a bit bogus since it actually is a for statement in
> disguise.  The following patch adds list_for_each to the list of things
> that look like functions that it shouldn't complain about.

We have had some stabs at changing this, but no consensus was reached on
whether it was a "for" or a "function".  My memory is of there being
slightly more "without a space" tenders than the other and so it has not
been changed.  This thread also seems so far to have not really
generated a concensus.  So I would tend to leave things as they are.  

A third option might be to accept either on *for_each* constructs.
That might tend to lead to divergance.  Difficult.  However, also see my
later comments on "style guide".

> 
> By the way, what is the consensus on lines over 80 characters?
> checkpatch complains about the following:
> 
> WARNING: line over 80 characters
> #762: FILE: drivers/spi/spi_s3c24xx_dma.c:720:
> +       printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav Technologies AB\n");
> 
> I can of course break this into:
> 
>         printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav "
> 	       "Technologies AB\n");
> 
> but in my opinion that becomes more even unreadable.  Would it be
> possible to add a special case so that checkpatch ignores long strings
> that go beyond 80 characters?  Do you think it is a good idea?

I think you are miss-understanding the point of checkpatch here.  It has
flagged this line as suspect.  You are happy it is better as it is.  You
should therefore leave it as it is as you "are happy to justify that
checkpatch failure".  Checkpatch is a style guide, if it complains you
should think about its complaint and act as you see fit in response.
Sometimes you will ignore it, that is fine.

-apw

Re: [PATCH] teach checkpatch.pl about list_for_each

[Christoph Hellwig]

On Thu, Jan 03, 2008 at 11:10:58AM +0000, Andy Whitcroft wrote:
> We have had some stabs at changing this, but no consensus was reached on
> whether it was a "for" or a "function".  My memory is of there being
> slightly more "without a space" tenders than the other and so it has not
> been changed.  This thread also seems so far to have not really
> generated a concensus.  So I would tend to leave things as they are.  
> 
> A third option might be to accept either on *for_each* constructs.
> That might tend to lead to divergance.  Difficult.  However, also see my
> later comments on "style guide".

Pretty much all core code uses list_for_each_entry( so new code should
follow that example.

Re: [PATCH] teach checkpatch.pl about list_for_each

[Arnaldo Carvalho de Melo]

Em Thu, Jan 03, 2008 at 12:26:10PM +0000, Christoph Hellwig escreveu:
> On Thu, Jan 03, 2008 at 11:10:58AM +0000, Andy Whitcroft wrote:
> > We have had some stabs at changing this, but no consensus was reached on
> > whether it was a "for" or a "function".  My memory is of there being
> > slightly more "without a space" tenders than the other and so it has not
> > been changed.  This thread also seems so far to have not really
> > generated a concensus.  So I would tend to leave things as they are.  
> > 
> > A third option might be to accept either on *for_each* constructs.
> > That might tend to lead to divergance.  Difficult.  However, also see my
> > later comments on "style guide".
> 
> Pretty much all core code uses list_for_each_entry( so new code should
> follow that example.

Agreed, CodingStyle is not about mindless consistency such as "for (" is
the right thing, so "list_for_each (" is consistent with it, it is about
codifying practice contributors got used to over the years.

- Arnaldo

Re: [PATCH] teach checkpatch.pl about list_for_each

[Benny Halevy]

On Jan. 03, 2008, 14:30 +0200, Arnaldo Carvalho de Melo <acme@redhat.com> wrote:
> Em Thu, Jan 03, 2008 at 12:26:10PM +0000, Christoph Hellwig escreveu:
>> On Thu, Jan 03, 2008 at 11:10:58AM +0000, Andy Whitcroft wrote:
>>> We have had some stabs at changing this, but no consensus was reached on
>>> whether it was a "for" or a "function".  My memory is of there being
>>> slightly more "without a space" tenders than the other and so it has not
>>> been changed.  This thread also seems so far to have not really
>>> generated a concensus.  So I would tend to leave things as they are.  
>>>
>>> A third option might be to accept either on *for_each* constructs.
>>> That might tend to lead to divergance.  Difficult.  However, also see my
>>> later comments on "style guide".
>> Pretty much all core code uses list_for_each_entry( so new code should
>> follow that example.
> 
> Agreed, CodingStyle is not about mindless consistency such as "for (" is
> the right thing, so "list_for_each (" is consistent with it, it is about
> codifying practice contributors got used to over the years.
> 

Why mindless?
Coding style is also about giving the coding language logic a graphical
representation.  Following a convention that flow control keywords
such as "if", "for", or "while" are distinguished from function calls
by use of a space after the keyword really helps the code readability
regardless of how people used to code it in the past...
The for_each_* macros are clearly not function calls but rather translate
to for () flow control constructs hence they should follow the same convention.
FWIW, I think that changing the existing convention is worth it in this case.

Benny

> - Arnaldo

Re: [PATCH] teach checkpatch.pl about list_for_each

[Christer Weinigel]

On Thu, 03 Jan 2008 17:17:29 +0200
Benny Halevy <bhalevy@panasas.com> wrote:

> On Jan. 03, 2008, 14:30 +0200, Arnaldo Carvalho de Melo
> > Agreed, CodingStyle is not about mindless consistency such as "for
> > (" is the right thing, so "list_for_each (" is consistent with it,
> > it is about codifying practice contributors got used to over the
> > years.
> > 
> 
> Why mindless?
> Coding style is also about giving the coding language logic a
> graphical representation.  Following a convention that flow control
> keywords such as "if", "for", or "while" are distinguished from
> function calls by use of a space after the keyword really helps the
> code readability regardless of how people used to code it in the
> past... The for_each_* macros are clearly not function calls but
> rather translate to for () flow control constructs hence they should
> follow the same convention. FWIW, I think that changing the existing
> convention is worth it in this case.

Definite agreement here, since _for_each is used for flow control, that
space should be there.  

And some people seem to take checkpatch.pl as the gospel, and won't
apply code with checkpatch warnings.

  /Christer

Re: [PATCH] teach checkpatch.pl about list_for_each

[pHilipp Zabel]

On Dec 2, 2007 1:03 PM, Christer Weinigel <christer@weinigel.se> wrote:
> Hi Andy,
>
> you seem to be the last person messing around with checkpatch.pl so I'm
> addressing this to you. :-)
>
> checkpatch complains about the following:
>
> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> +       list_for_each_entry (transfer, &message->transfers, transfer_list) {
>
> which I think is a bit bogus since it actually is a for statement in
> disguise.  The following patch adds list_for_each to the list of things
> that look like functions that it shouldn't complain about.
>
> By the way, what is the consensus on lines over 80 characters?
> checkpatch complains about the following:
>
> WARNING: line over 80 characters
> #762: FILE: drivers/spi/spi_s3c24xx_dma.c:720:
> +       printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav Technologies AB\n");
>
> I can of course break this into:
>
>         printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav "
>                "Technologies AB\n");

I'd not split it in the middle of a name or phrase if possible.
        printk(KERN_INFO "S3C24xx SPI DMA driver"
                "(c) 2007 Nordnav Technologies AB\n");
but ...

> but in my opinion that becomes more even unreadable.  Would it be
> possible to add a special case so that checkpatch ignores long strings
> that go beyond 80 characters?  Do you think it is a good idea?

... in this case
       pr_info("S3C24xx SPI DMA driver (c) 2007 Nordnav Technologies AB\n");
might just push it under the limit.

cheers
Philipp

Re: [PATCH] teach checkpatch.pl about list_for_each

[Tomas Carnecky]

Christer Weinigel wrote:
> By the way, what is the consensus on lines over 80 characters?
> checkpatch complains about the following:
> 
> WARNING: line over 80 characters
> #762: FILE: drivers/spi/spi_s3c24xx_dma.c:720:
> +       printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav Technologies AB\n");
> 
> I can of course break this into:
> 
>         printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav "
> 	       "Technologies AB\n");
> 
> but in my opinion that becomes more even unreadable.  Would it be
> possible to add a special case so that checkpatch ignores long strings
> that go beyond 80 characters?  Do you think it is a good idea?

At the top of the file add a #define and use that in the code? Some 
drivers define their version/author etc that way and then just
printk(DRIVER_VERSION DRIVER_AUTHOR);

tom

Re: [PATCH] teach checkpatch.pl about list_for_each

[Christer Weinigel]

On Thu, 03 Jan 2008 13:34:45 +0100
Tomas Carnecky <tom@dbservice.com> wrote:

> Christer Weinigel wrote:
> > By the way, what is the consensus on lines over 80 characters?
> > checkpatch complains about the following:
> > 
> > WARNING: line over 80 characters
> > #762: FILE: drivers/spi/spi_s3c24xx_dma.c:720:
> > +       printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav
> > Technologies AB\n");
> > 
> > I can of course break this into:
> > 
> >         printk(KERN_INFO "S3C24xx SPI DMA driver (c) 2007 Nordnav "
> > 	       "Technologies AB\n");
> > 
> > but in my opinion that becomes more even unreadable.  Would it be
> > possible to add a special case so that checkpatch ignores long
> > strings that go beyond 80 characters?  Do you think it is a good
> > idea?
> 
> At the top of the file add a #define and use that in the code? Some 
> drivers define their version/author etc that way and then just
> printk(DRIVER_VERSION DRIVER_AUTHOR);

That only solves this specific problem.  For debugging printks, which
often become quite wide, it would make the code even more unreadable to
add lots of defines just to keep things within 80 cols.

  /Christer