teach checkpatch.pl about list_for_each
PATCH 1/1 teach checkpatch.pl about list_for_each
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
scripts/checkpatch.pl +1 -1
@@ -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); }
Em Sun, Dec 02, 2007 at 01:03:35PM +0100, Christer Weinigel escreveu:
Quoted 7 lines
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:
Quoted 7 lines
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_eachto 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
Quoted 6 lines · Christer Weinigel
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.
:)
Quoted 7 lines · 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:
Quoted 7 lines
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_eachto 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".
Quoted 8 lines
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");
Quoted 8 lines
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
Quoted 9 lines · Andy Whitcroft
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.
Em Thu, Jan 03, 2008 at 12:26:10PM +0000, Christoph Hellwig escreveu:
Quoted 7 lines · Andy Whitcroft
Quoted 6 lines
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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 11 lines · Arnaldo Carvalho de Melo
Em Thu, Jan 03, 2008 at 12:26:10PM +0000, Christoph Hellwig escreveu:
Quoted 10 lines · Andy Whitcroft
Quoted 9 lines
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".
Quoted 7 lines
Quoted 2 lines
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
Quoted 1 line
- Arnaldo
Quoted 7 lines · On Thu, 03 Jan 2008 17:17:29 +0200 Benny Halevy <[email protected]> wrote:
On Jan. 03, 2008, 14:30 +0200, Arnaldo Carvalho de Melo
Quoted 5 lines
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.
Quoted 10 lines
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
Quoted 18 lines · 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_eachto 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:
Quoted 8 lines
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 ...
Quoted 3 lines
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
Quoted 7 lines · Christer Weinigel
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");
Quoted 8 lines
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
Quoted 14 lines · On Thu, 03 Jan 2008 13:34:45 +0100 Tomas Carnecky <[email protected]> wrote:
Quoted 13 lines · Christer Weinigel
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");
Quoted 8 lines
Quoted 4 lines
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