Re: [PATCH] teach checkpatch.pl about list_for_each
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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Thu, 03 Jan 2008 13:34:45 +0100 Tomas Carnecky <[EMAIL PROTECTED]> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Jan. 03, 2008, 14:30 +0200, Arnaldo Carvalho de Melo <[EMAIL PROTECTED]> wrote: > Em Thu, Jan 03, 2008 at 12:26:10PM +, Christoph Hellwig escreveu: >> On Thu, Jan 03, 2008 at 11:10:58AM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
Em Thu, Jan 03, 2008 at 12:26:10PM +, Christoph Hellwig escreveu: > On Thu, Jan 03, 2008 at 11:10:58AM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Thu, Jan 03, 2008 at 11:10:58AM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Dec 2, 2007 1:03 PM, Christer Weinigel <[EMAIL PROTECTED]> 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, >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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, >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Dec 2, 2007 1:03 PM, Christer Weinigel [EMAIL PROTECTED] 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Thu, Jan 03, 2008 at 11:10:58AM +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
Em Thu, Jan 03, 2008 at 12:26:10PM +, Christoph Hellwig escreveu: On Thu, Jan 03, 2008 at 11:10:58AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Jan. 03, 2008, 14:30 +0200, Arnaldo Carvalho de Melo [EMAIL PROTECTED] wrote: Em Thu, Jan 03, 2008 at 12:26:10PM +, Christoph Hellwig escreveu: On Thu, Jan 03, 2008 at 11:10:58AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
On Thu, 03 Jan 2008 13:34:45 +0100 Tomas Carnecky [EMAIL PROTECTED] 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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, >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. :) pgp0p0nA2PLDM.pgp Description: PGP signature
Re: [PATCH] teach checkpatch.pl about list_for_each
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, >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: [EMAIL PROTECTED] net-2.6.25]$ find . -name "*.[ch]" | xargs grep '_for_each[a-z_]*(' | wc -l 4370 [EMAIL PROTECTED] net-2.6.25]$ find . -name "*.[ch]" | xargs grep '_for_each[a-z_]* (' | wc -l 160 [EMAIL PROTECTED] 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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, >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); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 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); } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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: [EMAIL PROTECTED] net-2.6.25]$ find . -name *.[ch] | xargs grep '_for_each[a-z_]*(' | wc -l 4370 [EMAIL PROTECTED] net-2.6.25]$ find . -name *.[ch] | xargs grep '_for_each[a-z_]* (' | wc -l 160 [EMAIL PROTECTED] 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] teach checkpatch.pl about list_for_each
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. :) pgp0p0nA2PLDM.pgp Description: PGP signature